module: allow ESM that failed to be required to be re-imported #55502
Conversation
This file contains 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
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails.
|
Review requested:
|
nodejs-github-bot
added
esm
github-actions
bot
removed
the
request-ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55502 +/- ##
==========================================
- Coverage 88.41% 88.41% -0.01%
==========================================
Files 653 653
Lines 187476 187492 +16
Branches 36083 36089 +6
==========================================
+ Hits 165763 165769 +6
- Misses 14946 14960 +14
+ Partials 6767 6763 -4
|
There was a problem hiding this comment.
Question: what is the status of a module that has top-level await when it is done with the initial synchronous part but it's still waiting for the tla to complete?
try {
require("./tla.js");
} catch {
import("./tla.js");
// what is the status here, when this import starts?
import("./tla.js");
}// tla.js
await new Promise(resolve => setTimeout(resolve, 1000));
It is kEvaluated because currently V8 coerce kEvaluatingAsync to kEvaluated. |
|
Added a couple more tests for the evaluating phase. We should also deal with kErrored, so changed the status check a little and await on the evaluation promise. |
github-actions
bot
removed
the
request-ci
atlowChemi
approved these changes
Oct 26, 2024
github-actions
bot
removed
the
request-ci
joyeecheung
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 7cb3a66 |
Ceres6
pushed a commit
to Ceres6/node
that referenced
this pull request
Oct 30, 2024
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: nodejs#55502 Fixes: nodejs#55500 Refs: nodejs#52697 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
RafaelGSS
pushed a commit
that referenced
this pull request
Nov 1, 2024
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: #55502 Fixes: #55500 Refs: #52697 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
louwers
pushed a commit
to louwers/node
that referenced
this pull request
Nov 2, 2024
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: nodejs#55502 Fixes: nodejs#55500 Refs: nodejs#52697 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
joyeecheung
added a commit
to joyeecheung/node
that referenced
this pull request
Nov 12, 2024
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: nodejs#55502 Fixes: nodejs#55500 Refs: nodejs#52697 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
joyeecheung
added a commit
to joyeecheung/node
that referenced
this pull request
Nov 12, 2024
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: nodejs#55502 Fixes: nodejs#55500 Refs: nodejs#52697 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
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.
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails.
Fixes: #55500
Refs: #52697