Skip to content

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 guybedford force-pushed the main-skip-preserve-symlinks branch from 81003ef to 292c7bd Compare March 17, 2018 17:37
@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 guybedford force-pushed the main-skip-preserve-symlinks branch from 292c7bd to a9dc4f8 Compare March 20, 2018 14:42
@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 guybedford force-pushed the main-skip-preserve-symlinks branch from 8991bcb to 994ed4e Compare March 31, 2018 12:48
@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