-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
[v20.x] backport require(esm) bugfixes and improvements #59504
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
Closed
joyeecheung
wants to merge
9
commits into
nodejs:v20.x-staging
from
joyeecheung:backport-require-esm-patches
Closed
[v20.x] backport require(esm) bugfixes and improvements #59504
joyeecheung
wants to merge
9
commits into
nodejs:v20.x-staging
from
joyeecheung:backport-require-esm-patches
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Review requested:
|
mcollina
approved these changes
Aug 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rslgtm
@joyeecheung there are some lint errors |
2bd8221
to
c059eb2
Compare
Fixed the linter error |
a2554ef
to
5a3b04b
Compare
marco-ippolito
approved these changes
Aug 18, 2025
I think it just requires a rebase since I pushed on staging |
c059eb2
to
a65b63e
Compare
Rebased. |
5a3b04b
to
d65a24c
Compare
This refactors the CommonJS loading a bit to create a center point that handles source loading (`loadSource`) and make format detection more consistent to pave the way for future synchronous hooks. - Handle .mjs in the .js handler, similar to how .cjs has been handled. - Generate the legacy ERR_REQUIRE_ESM in a getRequireESMError() for require(esm) handling (when it's disabled). PR-URL: nodejs#55590 Refs: nodejs/loaders#198 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
If the macros are used as ERR_*(isolate, message) or THROW_ERR_*(isolate, message) with a single string argument, do run formatter on the message, and allow the caller to pass in a message directly with characters that would otherwise need escaping if used as format string unconditionally. PR-URL: nodejs#57126 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
- Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time. - Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available. Drive-by: split the require(tla) tests since we are modifying the tests already. PR-URL: nodejs#57126 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: nodejs#57187 Fixes: nodejs#57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Fixes: nodejs#57451 PR-URL: nodejs#57453 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When require(esm) encounters a cached module job that is instantiated but not yet evaluated, run the evaluation. This catches an edge case previously missed in nodejs#57187. PR-URL: nodejs#58067 Fixes: nodejs#58061 Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: nodejs#56491 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>
When --import is used, the ESM loader is used to handle even pure CJS entry points, and it can run into CJS module facades in the evaluating state when the parent CJS module is being evaluated. In this case it should be allowed, since the ESM <-> CJS cycles that are meant to be disallowed (for the time being) should already be detected before evaluation and wouldn't get here, and CJS <-> CJS cycles are fine. PR-URL: nodejs#58598 Fixes: nodejs#58515 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs. PR-URL: nodejs#58957 Fixes: nodejs#58945 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 25, 2025
When --import is used, the ESM loader is used to handle even pure CJS entry points, and it can run into CJS module facades in the evaluating state when the parent CJS module is being evaluated. In this case it should be allowed, since the ESM <-> CJS cycles that are meant to be disallowed (for the time being) should already be detected before evaluation and wouldn't get here, and CJS <-> CJS cycles are fine. PR-URL: #58598 Backport-PR-URL: #59504 Fixes: #58515 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 25, 2025
Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs. PR-URL: #58957 Backport-PR-URL: #59504 Fixes: #58945 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 25, 2025
This refactors the CommonJS loading a bit to create a center point that handles source loading (`loadSource`) and make format detection more consistent to pave the way for future synchronous hooks. - Handle .mjs in the .js handler, similar to how .cjs has been handled. - Generate the legacy ERR_REQUIRE_ESM in a getRequireESMError() for require(esm) handling (when it's disabled). PR-URL: #55590 Backport-PR-URL: #59504 Refs: nodejs/loaders#198 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 25, 2025
If the macros are used as ERR_*(isolate, message) or THROW_ERR_*(isolate, message) with a single string argument, do run formatter on the message, and allow the caller to pass in a message directly with characters that would otherwise need escaping if used as format string unconditionally. PR-URL: #57126 Backport-PR-URL: #59504 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 25, 2025
- Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time. - Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available. Drive-by: split the require(tla) tests since we are modifying the tests already. PR-URL: #57126 Backport-PR-URL: #59504 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 25, 2025
This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: #57187 Backport-PR-URL: #59504 Fixes: #57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 25, 2025
Fixes: #57451 PR-URL: #57453 Backport-PR-URL: #59504 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 25, 2025
When --import is used, the ESM loader is used to handle even pure CJS entry points, and it can run into CJS module facades in the evaluating state when the parent CJS module is being evaluated. In this case it should be allowed, since the ESM <-> CJS cycles that are meant to be disallowed (for the time being) should already be detected before evaluation and wouldn't get here, and CJS <-> CJS cycles are fine. PR-URL: #58598 Backport-PR-URL: #59504 Fixes: #58515 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 25, 2025
Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs. PR-URL: #58957 Backport-PR-URL: #59504 Fixes: #58945 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 26, 2025
This refactors the CommonJS loading a bit to create a center point that handles source loading (`loadSource`) and make format detection more consistent to pave the way for future synchronous hooks. - Handle .mjs in the .js handler, similar to how .cjs has been handled. - Generate the legacy ERR_REQUIRE_ESM in a getRequireESMError() for require(esm) handling (when it's disabled). PR-URL: #55590 Backport-PR-URL: #59504 Refs: nodejs/loaders#198 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 26, 2025
If the macros are used as ERR_*(isolate, message) or THROW_ERR_*(isolate, message) with a single string argument, do run formatter on the message, and allow the caller to pass in a message directly with characters that would otherwise need escaping if used as format string unconditionally. PR-URL: #57126 Backport-PR-URL: #59504 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 26, 2025
- Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time. - Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available. Drive-by: split the require(tla) tests since we are modifying the tests already. PR-URL: #57126 Backport-PR-URL: #59504 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 26, 2025
This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: #57187 Backport-PR-URL: #59504 Fixes: #57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 26, 2025
Fixes: #57451 PR-URL: #57453 Backport-PR-URL: #59504 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 26, 2025
When --import is used, the ESM loader is used to handle even pure CJS entry points, and it can run into CJS module facades in the evaluating state when the parent CJS module is being evaluated. In this case it should be allowed, since the ESM <-> CJS cycles that are meant to be disallowed (for the time being) should already be detected before evaluation and wouldn't get here, and CJS <-> CJS cycles are fine. PR-URL: #58598 Backport-PR-URL: #59504 Fixes: #58515 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 26, 2025
Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs. PR-URL: #58957 Backport-PR-URL: #59504 Fixes: #58945 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 27, 2025
This refactors the CommonJS loading a bit to create a center point that handles source loading (`loadSource`) and make format detection more consistent to pave the way for future synchronous hooks. - Handle .mjs in the .js handler, similar to how .cjs has been handled. - Generate the legacy ERR_REQUIRE_ESM in a getRequireESMError() for require(esm) handling (when it's disabled). PR-URL: #55590 Backport-PR-URL: #59504 Refs: nodejs/loaders#198 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 27, 2025
If the macros are used as ERR_*(isolate, message) or THROW_ERR_*(isolate, message) with a single string argument, do run formatter on the message, and allow the caller to pass in a message directly with characters that would otherwise need escaping if used as format string unconditionally. PR-URL: #57126 Backport-PR-URL: #59504 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 27, 2025
- Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time. - Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available. Drive-by: split the require(tla) tests since we are modifying the tests already. PR-URL: #57126 Backport-PR-URL: #59504 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 27, 2025
This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: #57187 Backport-PR-URL: #59504 Fixes: #57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 27, 2025
Fixes: #57451 PR-URL: #57453 Backport-PR-URL: #59504 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 27, 2025
When --import is used, the ESM loader is used to handle even pure CJS entry points, and it can run into CJS module facades in the evaluating state when the parent CJS module is being evaluated. In this case it should be allowed, since the ESM <-> CJS cycles that are meant to be disallowed (for the time being) should already be detected before evaluation and wouldn't get here, and CJS <-> CJS cycles are fine. PR-URL: #58598 Backport-PR-URL: #59504 Fixes: #58515 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Refs: #52697
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 27, 2025
Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs. PR-URL: #58957 Backport-PR-URL: #59504 Fixes: #58945 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Refs: #52697
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This backports the following PRs to v20.x
Refs: #52697