Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

when calling default* functions, you should not recursively pass default* function #1849

Closed

Conversation

giltayar
Copy link

According to the documentation (https://nodejs.org/api/esm.html#loaders), when calling nextResolve
and nextLoad, one should not pass the next* function again to itself.

This causes a problem when using multiple loaders, as evidenced by this issue: testdouble/testdouble.js#496.

Two notes:

  1. Not sure exactly what the scenario is, so I couldn't figure out what test to write. All I know is that
    in the reproduction, when I removed ts-node's recursive call, everything worked.
  2. Now that the parameter defaultResolve/load signature is not the same as resolve/load-s,
    I made it any. Any other try created errors in other places. Sorry, I'm not a TS expert.

@giltayar giltayar marked this pull request as ready for review July 26, 2022 16:14
@cspotcode
Copy link
Collaborator

Thanks, do we think this is a bug in node itself? It seems strange to me that node would be paying attention to this third argument at all; node should be guarding against this argument having any effect.

Typically, well-behaved JS APIs do not change behavior when passed an undocumented argument. Documented arguments, of course, can change behavior.

This makes me suspect something fishy is happening in node's loader chaining logic.

@giltayar
Copy link
Author

@cspotcode: from what I've seen, this changed in 18.6.0, when they added multiple loader support. Before you could call with the third argument and it worked.

It's not fishy: it's not really calling the next resolution in the chain, but rather going through some code of their own. But you're right in that if the function should be called with two arguments, then they should be totally ignoring the third argument!

But what can I say 🤷‍♂️? In the end, the documentation says that you should not pass the third argument, so the fix is still a correct one, and it fixes problems when this loader is in a chain.

@cspotcode
Copy link
Collaborator

cspotcode commented Jul 26, 2022

I've raised this with the loaders team on Slack. Proper bugfix should happen in node, so we'll pursue that route. In the meantime, downgrading node is always an option.

It's an unwritten rule1 of JS, that functions implicitly agree not to change behavior when passed additional arguments, unless they're documented to take rest args. The node code in question was actually flagged in code review, so seems likely that my report will cause them to fix this. The code is also checking arguments.length instead of === undefined, which is another code smell.

Footnotes

  1. "Rule" meaning we should follow it because it makes everyone happier, not referring to any tangible enforcement mechanism within the runtime, linters, etc.

@giltayar
Copy link
Author

@cspotcode your call, of course! Feel free to close this PR without merging it, because in the end you are right and this is a Node.js bug/smell. But from what I've seen, removing that third argument does not harm in any version of Node, so it's a benign change.

@cspotcode
Copy link
Collaborator

Got confirmation on Slack that this will be fixed in node. I've added a note to fix the types here: #1505 (that issue is titled breaking changes, but even if this types change isn't breaking, our next version is going to be a major, so that was an easy place to write it down)

I meant to dig up another example of why functions shouldn't rely on arguments.length nor change behavior when passed undocumented args: Array.prototype.map

E.g. it's common to see function mapStuff(input) {return input.foo ? true : false} and then do items.map(mapStuff).
Additional args are being passed, but that's ok, because mapStuff's documentation says that it accepts a single arg, so the unwritten expectation is that it'll accept a second and third arg without breaking.

@cspotcode
Copy link
Collaborator

Fixed by nodejs/node#44109

@cspotcode cspotcode closed this Aug 5, 2022
@giltayar giltayar deleted the no-recursive-esm-next-call branch August 8, 2022 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants