module: fix detect-module not retrying as ESM for code that errors only in CommonJS #52024
Conversation
GeoffreyBooth
added
module
nodejs-github-bot
added
c++
GeoffreyBooth
force-pushed
the
fix-detect-module-cjs-exceptions
branch
2 times, most recently
from
3b2f28e to
680694b
Compare
|
I might not be reading the code correctly, but does this cover the case of: await Promise.resolve();
import 'x';It looks like the C++ code would try to handle the "await" syntax error first, wrap the code in an async function and recompile. The recompilation would then throw an "import" syntax error, ultimately causing |
Great catch @chjj, I’ve updated the PR to address that case. Any others that you can think of that I might have missed? |
There was a problem hiding this comment.
This is looking good! const module = 'x' as defining an ES module as an extension of the ESM syntax checks seems like the right kind of compromise to be making in the design space to me. I do hope we could optimize TLA in due course.
Note that when sloppy mode errors precede an identifier redeclaration there will still be error consistency, but as discussed separately there are ways to handle this by pointing to the correct error for the ESM parse, and this can be done as follow-up work if needed.
I would be happy to mark for approval as soon as we've got an updated note in the ESM docs describing the new format detection rules, as I think it's important to track the rules and their changes carefully.
Added. |
GeoffreyBooth
added
the
author ready
GeoffreyBooth
force-pushed
the
fix-detect-module-cjs-exceptions
branch
from
93c2b94 to
3fbeb43
Compare
…round async function wrapper; add test
GeoffreyBooth
force-pushed
the
fix-detect-module-cjs-exceptions
branch
from
3fbeb43 to
228f5c7
Compare
GeoffreyBooth
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 63d04d4 |
PR-URL: nodejs#52024 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: #52024 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: #52024 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: nodejs#52024 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
Fixes #50917, using the solution described in #50917 (comment).
The general algorithm for
--experimental-detect-moduleis to try to parse as CommonJS, and if a syntax error is thrown that corresponds to ESM syntax (importorexport, orimport.meta), try again as ESM. The edge case pointed out by #50917 is when there’s syntax that throws in CommonJS but parses in ESM, and this syntax is above the first ESM syntax (so top-levelawait, or a declaration of a variable with the same name as one of the CommonJS module wrapper variables such asconst require =, on the first line or any line above the firstimportorexport).The tricky thing is that the errors thrown by top-level
awaitorconst requirein CommonJS are the same errors as thrown by typingawaitin any ordinary sync function, or by declaringrequiretwice in user code (e.g.const require = 1; const require = 2). So we only want to retry parsing as ESM when these errors are because theawaitor problematic variable declaration is at the top level, where it throws in CommonJS but parses in ESM.To determine this, this PR creates a new error path where if the CommonJS parse returns a syntax error corresponding to either of these cases, we do a special second CommonJS parse of the code wrapped in an async function. This wrapper function creates a new scope, so
const requireis no longer a problematic redeclaration; andawaitno longer throws because it’s within an async function. This wrapper only affects the top level, soawaitin a sync function farther down or variable redeclarations farther down (or twoconstdeclarations at the top level) will still throw; but the code permitted in ESM parses successfully. If this second parse doesn’t throw any errors, we resume the detection algorithm and try parsing as ESM.@nodejs/loaders @chjj @meyfa @joyeecheung