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

esm: fix esm load bug #25491

Closed
wants to merge 3 commits into from
Closed

esm: fix esm load bug #25491

wants to merge 3 commits into from

Conversation

ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Jan 14, 2019

If module.reflect is undefined, just run into else condition.

Fixes: #25482

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@guybedford
Copy link
Contributor

Unfortunately we can't just override the module map - it has to be treated as an immutable map otherwise we can possibly get race conditions in loading / different instances.

The fix here has to be to ensure the onReady function is available as soon as the moduleMap entry is set.

@guybedford
Copy link
Contributor

(or change the architecture to not need an onReady)

@ZYSzys
Copy link
Member Author

ZYSzys commented Jan 14, 2019

So how should this to be fixed specifically ?Can you please give me some more guides ?

I've changed it like we previously did: https://github.com/nodejs/node/pull/24560/files#diff-76195ce57689942222a27f0dbda6d3b7L641

Sincerely /cc @devsnek who actual implement the refactor in #24560 (comment)

@devsnek
Copy link
Member

devsnek commented Jan 14, 2019

lite ci run to see if this fix passes https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2243/

@guybedford
Copy link
Contributor

One thing to try might be checking if reflect is available once module.modulePromise has resolved. It would be good to get a test case that captures what is happening in #25482 as well.

Thanks @ZYSzys for your efforts here.

@devsnek
Copy link
Member

devsnek commented Jan 14, 2019

the current change passes all tests, so maybe we're on to something?

cc @bmeck for some hopeful insight

@targos targos added the esm Issues and PRs related to the ECMAScript Modules implementation. label Apr 25, 2019
@guybedford
Copy link
Contributor

So the problem here is that the CJS cache injects into the ESM loader so they share the same instances, but in the case where the ESM loader has already loaded the module, the CJS version should not override the existing ESM version.

So the fix is correct.

I've merged the test case from #27443 into this PR, and can confirm it is fully resolved.

Please lets land this soon and backport the patch as well.

@guybedford guybedford added python PRs and issues that require attention from people who are familiar with Python. and removed python PRs and issues that require attention from people who are familiar with Python. labels May 12, 2019
@devsnek devsnek added the experimental Issues and PRs related to experimental features. label May 12, 2019
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd prefer we cherry-pick or am in the test to preserve authorship for the contributor who wrote it, unless there's a good reason not to do that.

ZYSzys and others added 3 commits May 13, 2019 17:37
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: nodejs#25482
@ZYSzys
Copy link
Member Author

ZYSzys commented May 13, 2019

@Trott I've cherry-pick the test and will squash the last fixup when landed, is it ok now ?

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor

Thanks @ZYSzys for updating that - of course we should retain the original commit.

ZYSzys added a commit that referenced this pull request May 14, 2019
Fixes: #25482

PR-URL: #25491
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
ZYSzys pushed a commit that referenced this pull request May 14, 2019
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: #25482

PR-URL: #25491
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@ZYSzys
Copy link
Member Author

ZYSzys commented May 14, 2019

Landed in 523a9fb...ac3b98c

@aduh95 Thanks and congratulate for your first(Oh, it's not your first time 😅, but still congratulate) contribution to Node.js core 🎉.

@ZYSzys ZYSzys closed this May 14, 2019
@ZYSzys ZYSzys deleted the fix-esm-bug branch May 14, 2019 08:57
targos pushed a commit that referenced this pull request May 14, 2019
Fixes: #25482

PR-URL: #25491
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request May 14, 2019
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: #25482

PR-URL: #25491
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
BethGriggs pushed a commit that referenced this pull request Jun 6, 2019
Fixes: #25482

Backport-PR-URL: #27874
PR-URL: #25491
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jun 6, 2019
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: #25482

Backport-PR-URL: #27874
PR-URL: #25491
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Jul 17, 2019
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
Fixes: nodejs/node#25482

PR-URL: nodejs/node#25491
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: nodejs/node#25482

PR-URL: nodejs/node#25491
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

esm: Bug in dynamic modules refactoring
8 participants