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.

Sign up for GitHub

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

Jump to bottom

Possible fix for #15374 #15389

Closed
wants to merge 1 commit into from
Closed

Possible fix for #15374 #15389

wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

This should fix the Windows path bug with drive letters being detected as the protocol, getting modules to work on Windows properly.

I still need to actually test this out on a Windows machine (won't have access again till tomorrow) but I'm 99% sure it'll work out.

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

esmodules

@nodejs-github-bot nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Sep 13, 2017
@targos
Copy link
Member

targos commented Sep 13, 2017

Thanks for the fix! If you are able to write a test for it, we can run it on CI and we'll see if it works on the Windows machines.

/cc @bmeck

@bmeck
Copy link
Member

bmeck commented Sep 13, 2017 via email

@mscdex mscdex added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 13, 2017
bzoz
bzoz approved these changes Sep 13, 2017
View reviewed changes
Copy link
Contributor

@bzoz bzoz left a comment

Choose a reason for hiding this comment

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

Tested this, it fixes #15374.

@guybedford
Copy link
Contributor Author

guybedford commented Sep 13, 2017 •
edited
Loading

Wrt to the tests, I'm not that knowledgable on how they actually get executed at a top level to know what the exact entry point is here, and as to why they weren't failing already on windows. Any end-to-end modules test should capture this - I don't think it's a specific test case.

@targos
Copy link
Member

targos commented Sep 13, 2017

@guybedford you are right. I just realized that es-module tests don't run on CI yet: #15276

@jdalton jdalton mentioned this pull request Sep 14, 2017
Closed
@MylesBorins
Copy link
Contributor

Since this has been manually verified to work should we land once 48 hours has passed? I don't think we should block on fixing the CI for this.

@jasnell
Copy link
Member

jasnell commented Sep 15, 2017

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

jasnell pushed a commit that referenced this pull request Sep 15, 2017
@guybedford @jasnell
module: check file url passed to top-level import
dce72c2 
Fixes: #15374
PR-URL: #15389
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 15, 2017

Landed in dce72c2

@jasnell jasnell closed this Sep 15, 2017
styfle
styfle reviewed Sep 15, 2017
View reviewed changes
lib/module.js
@@ -435,7 +435,7 @@ Module._load = function(request, parent, isMain) {
if (experimentalModules) {
if (filename === null || /\.mjs$/.test(filename)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why regex is used here instead of filename.endsWith('.mjs')?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bmeck?

Copy link
Member

Choose a reason for hiding this comment

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

Habits from before that existed. Technically we could/should not be using prototype functions since they could be tainted by preloading via --require.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.

Copy link
Member

@jdalton jdalton Sep 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Technically we could/should not be using prototype functions since they could be tainted by preloading

There's a lot to tweak then because there's heavy use of apply, call, charAt, concat, pop, push, slice, test, and on and on. It'd be neat to see if some of this could be solved with a new context to avoid plucking all these methods off.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot to tweak then because there's heavy use of charAt, concat, pop, push, test, slice, and on and on.

This concern pops up now and then.
Refs: #12956
Refs: #12981

Copy link
Member

Choose a reason for hiding this comment

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

@jdalton Contexts are expensive w/o snapshots to create and we would need a frozen one to avoid mutations which would mean making that custom Snapshot, we couldn't use vm.createContext due to the funky sandbox bridge it creates leaking : #6283 . I'd be open to using one if we had a snapshot, but it would still be ~10ms on my benchmarks to create a full one.

addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
@guybedford @addaleax
module: check file url passed to top-level import
587d3ae 
Fixes: nodejs/node#15374
PR-URL: nodejs/node#15389
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
@guybedford @jasnell
module: check file url passed to top-level import
c005713 
Fixes: #15374
PR-URL: #15389
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@jasnell jasnell mentioned this pull request Sep 20, 2017
Merged
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@guybedford
module: check file url passed to top-level import
ae1fdac 
Fixes: nodejs/node#15374
PR-URL: nodejs/node#15389
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdalton jdalton jdalton left review comments

@refack refack refack approved these changes

@styfle styfle styfle left review comments

@bmeck bmeck bmeck approved these changes

@jasnell jasnell jasnell approved these changes

@targos targos targos approved these changes

@daxlab daxlab daxlab approved these changes

@bzoz bzoz bzoz approved these changes

Assignees
No one assigned
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.