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: skip preserveSymlinks for main #19388

Closed
wants to merge 3 commits into from
Closed

module: skip preserveSymlinks for main #19388

wants to merge 3 commits into from

Conversation

guybedford
Copy link
Contributor

The CommonJS module resolver specifically skips --preserveSymlinks for the main entry point into Node.

From discussion in #19383, it seems like this should probably be carried over into the ES module resolver in order to ensure full backwards-compatibility.

The alternative here might be to have --preserveSymlinks act on the main in the CJS resolver too instead of this PR.

//cc @nodejs/modules

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@guybedford
Copy link
Contributor Author

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

@guybedford
Copy link
Contributor Author

Not sure if this is due to flaky CI or not... will try again once other CI jobs are looking greener.

@guybedford
Copy link
Contributor Author

guybedford commented Mar 20, 2018 •
edited
Loading

Turns out there was an issue with symlinks on Windows in fixtures, which should be resolved now.

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

@devsnek
Copy link
Member

devsnek commented Mar 20, 2018 •
edited
Loading

for the commit name, should the subsystem be esmodule or loader or something? module makes me think it touches the cjs module stuff

@guybedford
Copy link
Contributor Author

CI error this time seems to be completely unrelated, so I believe this is good to merge.

@guybedford
Copy link
Contributor Author

@devsnek esmodules isn't formally a subsystem I don't believe, although perhaps it should be? That said modules makes sense to me for both too.

@guybedford guybedford mentioned this pull request Mar 28, 2018
Closed
@guybedford
Copy link
Contributor Author

Rebased, latest CI: https://ci.nodejs.org/job/node-test-pull-request/13962/

Will actually merge this time.

@benjamingr
Copy link
Member

@guybedford forgot to merge :D ?

guybedford added a commit that referenced this pull request Apr 1, 2018
@guybedford
module: skip preserveSymlinks for main
141be92 
PR-URL: #19388
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@guybedford
Copy link
Contributor Author

Merged in 141be92.

@guybedford guybedford closed this Apr 1, 2018
targos pushed a commit that referenced this pull request Apr 2, 2018
@guybedford @targos
module: skip preserveSymlinks for main
859b719 
PR-URL: #19388
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Apr 4, 2018
Merged
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@guybedford @devsnek @benjamingr @jasnell