incorrect behavior when invalid number of arguments passed
DerekNonGeneric opened this issue · 6 comments
Since this package is widely used and advertised as being perfectly spec-compliant, feedback would be appreciated on nodejs/node#41450, which seems to make the case that a TypeError should be thrown in the event that no arguments are passed to these utility methods. Does that seem to align w/ your interpretation of the spec(s) governing the behavior of these utility methods?
We'd be interested in seeing whether we can have web-platform-test(s) added covering this case, which we would be able to run as part of the Node.js core WPT test suites. We are already pulling in some WPT tests via test/common/wpt.js (i.e., test/wpt/test-atob.js, but can easily include any additional tests covering an invalid number of arguments being passed or whatever else is added, which other projects like this one may similarly be able to use for their conformance testing.
You're right, this looks like a bug. Would you like to submit a pull request to fix this?
I am not currently a user of this module and only found out about it today by accident, but if you would be able to specify (or at least help clarify) what criteria would qualify as a fix (perhaps so that we can have it in the form of a conformance test in the web-platform-tests repo), I would gladly ensure that the fix gets implemented here as well as in the Node core repo.
So, I guess what I'm really interested in knowing is “what should we be asking this TypeError object” that would prove that we are now conforming to the spec?
This is not as easy of a question to answer as one may think, for example, we have varying degrees of similarity between error objects as can be seen in this expectedException assertion function we have in core.
This is already testsed in web platform tests: see https://wpt.fyi/results/html/dom/idlharness.https.html%3Finclude%3D(Document%7CWindow)?label=master&label=experimental&aligned&q=idlharness "Window interface: calling atob(DOMString) on window with too few arguments must throw TypeError"
The fix for this is just to check if the arguments array-like object within these functions has a length less than 1 and to throw a TypeError if they do, but am unclear on the exact error message text that would be desired (if at all). I am also unclear on what should happen if too many arguments are passed (more than 1) — TypeError with a different error message I suppose.
After reading the CONTRIBUTING.md, I think it may be best if maybe @jeffcarp or perhaps one of the owners of this repo were to do this instead of myself but am willing to make a PR attempting to fix this with any encouragement from you all.
Thanks @DerekNonGeneric & all. Happy to send a patch for this.
am unclear on the exact error message text
idlharness does not test the exact error message. As far as I can tell, re: nodejs/node#41450 (comment), there is no specification for the wording of this error message. We can make it match that of a browser implementation (nodejs/node#41450 (comment)).
I am also unclear on what should happen if too many arguments are passed
Browser implementations throw away any extra args. The spec seems to confirm this:
If there are more arguments passed in than the longest overload argument list, then they are ignored.