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

module: fixes #16476 ensuring extension lookups for top main #16526

Closed
wants to merge 1 commit into from
Closed

module: fixes #16476 ensuring extension lookups for top main #16526

wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

The reason as stated in #16476 (comment) is that absolute URLs do not go through extension and index checks.

By switching to an absolute path, the resolver still applies extensions properly to the top-level main.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • 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 Oct 26, 2017
@guybedford guybedford mentioned this pull request Oct 26, 2017
Closed
MylesBorins
MylesBorins approved these changes Oct 26, 2017
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

addaleax
addaleax reviewed Oct 26, 2017
View reviewed changes
test/parallel/test-module-main-extensions.js Outdated

try {
execFileSync(node, ['--experimental-modules', 'test/es-module/test-esm-ok']);
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this ever throw an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The resolver can throw but it shouldn't... let me clean this up.

test/parallel/test-module-main-extensions.js Outdated
if (e.toString().match(/Error: Cannot find module/))
assert.fail('Expected top-level extension lookup to resolve.');
}
assert.ok(true);
Copy link
Member

Choose a reason for hiding this comment

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

This line isn’t going to do anything, right? Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just from a habit of ensuring there is at least one assertion in a test. Can remove.

cjihrig
cjihrig approved these changes Oct 26, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM once the test nits are addressed.

@guybedford
Copy link
Contributor Author

Tests updated, and tested on Windows too. Should be good for CI.

@targos
Copy link
Member

targos commented Oct 26, 2017

What about naming the file test-module-main-no-extension.js to better explain what it's testing?

@joyeecheung
Copy link
Member

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

@targos
Copy link
Member

targos commented Oct 28, 2017

Landed in cadc47f

@targos targos closed this Oct 28, 2017
targos pushed a commit that referenced this pull request Oct 28, 2017
@guybedford @targos
module: fix extension lookups for top-level main
cadc47f 
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: #16526
Fixes: #16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@guybedford guybedford deleted the esm-main-extensions branch October 28, 2017 14:21
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@guybedford @gibfahn
module: fix extension lookups for top-level main
a0effd3 
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: #16526
Fixes: #16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@guybedford @gibfahn
module: fix extension lookups for top-level main
e63624b 
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: #16526
Fixes: #16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
@guybedford @gibfahn
module: fix extension lookups for top-level main
415fb56 
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: #16526
Fixes: #16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@guybedford @Qard
module: fix extension lookups for top-level main
af2b416 
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: nodejs/node#16526
Fixes: nodejs/node#16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@guybedford @Qard
module: fix extension lookups for top-level main
88d1802 
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: nodejs/node#16526
Fixes: nodejs/node#16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins
Copy link
Contributor

Assuming this shouldn't land on v6.x

Please lmk if I'm mistken

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@guybedford @addaleax
module: fix extension lookups for top-level main
b0c99a3 
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: nodejs/node#16526
Fixes: nodejs/node#16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@MylesBorins MylesBorins MylesBorins approved these changes

@benjamingr benjamingr benjamingr approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
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.

None yet
8 participants
@guybedford @targos @joyeecheung @MylesBorins @addaleax @benjamingr @cjihrig @nodejs-github-bot