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

module: skip preserveSymlinks for main #19388

Closed

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

@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

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

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
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
PR-URL: #19388
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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
PR-URL: #19388
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants