Skip to content

Refactor experimental-modules check into runMain#21350

Closed
guybedford wants to merge 4 commits intonodejs:masterfrom
guybedford:runmain
Closed

Refactor experimental-modules check into runMain#21350
guybedford wants to merge 4 commits intonodejs:masterfrom
guybedford:runmain

Conversation

@guybedford
Copy link
Contributor

@guybedford guybedford commented Jun 15, 2018

This provides a slightly cleaner separation, moving the top-level ESM load to higher up in the bootstrap as it should be.

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 15, 2018

@guybedford build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/78/pipeline

@devsnek
Copy link
Member

devsnek commented Jun 15, 2018 •
edited
Loading

its worth noting this isn't just a refactor, it actually fixes a bug where the loader can't run cjs. i can't quite figure out the repro though.

edit:
the bug is that the translator for cjs calls to Module._load which checks isMain and if its true calls loader.import, but the module was already imported so it just dies there.

moving this code ensures that the loop never happens.

/cc @guybedford could you add a test for this?

@guybedford
Copy link
Contributor Author

guybedford commented Jun 15, 2018

@devsnek well spotted on the failure case. Note that the failure case also exposes another bug that duplicate import executions of module jobs don't seem to be properly deduped through job.run execution. It may be better to create a test for that and solve the general issue separately.

bmeck
bmeck approved these changes Jun 15, 2018
View reviewed changes
Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

would like to see a test per @devsnek if possible

@guybedford
Copy link
Contributor Author

guybedford commented Jun 15, 2018

(CI failure unrelated)

devsnek
devsnek reviewed Jun 15, 2018
View reviewed changes
test/es-module/test-esm-cjs-main.js Outdated
Copy link
Member

@devsnek devsnek Jun 15, 2018

Choose a reason for hiding this comment

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

isn't there like common.fixture or smth for this

@guybedford
Copy link
Contributor Author

guybedford commented Jun 15, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15485/

BridgeAR
BridgeAR approved these changes Jun 18, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR 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 it would be nice if the nit from @devsnek could be addressed before landing.

@devsnek devsnek mentioned this pull request Jun 20, 2018
Closed
@devsnek
Copy link
Member

devsnek commented Jun 20, 2018

@guybedford is this ready to land?

Fishrock123
Fishrock123 reviewed Jun 20, 2018
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Looks good aside from gus's comment

@addaleax addaleax mentioned this pull request Jun 20, 2018
Closed
@addaleax
Copy link
Member

addaleax commented Jun 20, 2018

This is being reported as a regression in 10.5.0 already, see e.g. nodejs/help#1349

I’ve pushed a commit addressing @devsnek’s nit + adding a common.mustCall() where I think we need it

CI: https://ci.nodejs.org/job/node-test-pull-request/15542/

@devsnek
Copy link
Member

devsnek commented Jun 20, 2018 •
edited
Loading

es-module/test-esm-error-cache.js is failing but i can't really figure out why...

@guybedford
Copy link
Contributor Author

guybedford commented Jun 21, 2018

@devsnek it turns out because of this bug that test wasn't actually running to completion, and the file path was incorrect. I've resolved this here now.

CI: https://ci.nodejs.org/job/node-test-pull-request/15548/

@guybedford
Copy link
Contributor Author

guybedford commented Jun 21, 2018

Merged in 883e1cd.

@guybedford guybedford closed this Jun 21, 2018
guybedford added a commit that referenced this pull request Jun 21, 2018
@guybedford
module: experimental modules runMain separation
883e1cd 
PR-URL: #21350
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Jun 22, 2018
@guybedford @targos
module: experimental modules runMain separation
5cd78ba 
PR-URL: #21350
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Jul 3, 2018
Merged
@snyk-bot snyk-bot mentioned this pull request Mar 18, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+3 more reviewers

@Fishrock123 Fishrock123 Fishrock123 left review comments

@devsnek devsnek devsnek approved these changes

@bmeck bmeck bmeck approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.