fix: update alert method signature to accept string type #2006
Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
| @@ -718,7 +718,7 @@ | |||
| "method": { | |||
| "alert": { | |||
| "overrideSignatures": [ | |||
| "alert(message?: any): void" | |||
| "alert(message?: string): void" | |||
There was a problem hiding this comment.
In this case we can just remove this whole method. But have you checked why this is added?
There was a problem hiding this comment.
I have checked it, and the message is required by default. But the alert works even if it isn't required.
There was a problem hiding this comment.
Really? It seems it has been like that forever without a documented reason though. c9dcdfd
Anyway, please remove the whole method overridding and it'll become string.
There was a problem hiding this comment.
Err, actually the spec makes it multiple overloads instead of using an optional param. For now let's just do this.
|
LGTM |
|
There was an issue merging, maybe try again saschanaz. Details |
|
MDN implies that passing in a non-string is okay: https://developer.mozilla.org/en-US/docs/Web/API/Window/alert#message Maybe it's fine to be stricter, but I am wondering if it's worth it. |
|
Passing a non-string is always okay for any string parameter because everything will implicitly be stringified. Except most objects have no overriden stringifier and will just give |
|
(and this follows prompt() and confirm() for consistency) |
|
In general I agree, but alert does explicitly mention the conversion while prompt and confirm don't, which is why I mention it. |
|
Those all use the same IDL DOMString though? What MDN says is just whatever the human editor decided to write, and given the documentations are not automated, it's very expected to not say super consistently for the same things. |
|
That's fair, I was just foreseeing someone going "I can't alert with number anymore!" |
|
That foreseeing is fair too, and perhaps someone may quote MDN the same way you did. I can't imagine an actual production-ready web page would have For now I'd vote for consistency, but we can still see whether this really breaks anything and reconsider. LGTM |
|
Merging because @saschanaz is a code-owner of all the changes - thanks! |
If that occurs I can add make it accept numbers |
…rosoft#2006)" This reverts commit 3263db8.
I have updated the alert method to take a string.
fixes #1626