FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: skip preserveSymlinks for main by guybedford · Pull Request #19388 · nodejs/node · GitHub

/ node Public

module: skip preserveSymlinks for main - #19388

Closed
guybedford wants to merge 3 commits into
nodejs:masterfrom
guybedford:main-skip-preserve-symlinks
ConversationCommits (3)ChecksFiles changed
Closed

module: skip preserveSymlinks for main#19388
guybedford wants to merge 3 commits into
nodejs:masterfrom
guybedford:main-skip-preserve-symlinks

Conversation

guybedford commented Mar 16, 2018

Copy link
Copy Markdown
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 force-pushed the main-skip-preserve-symlinks branch from 81003ef to 292c7bd Compare March 17, 2018 17:37

guybedford commented Mar 17, 2018

Copy link
Copy Markdown
Contributor Author

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

guybedford commented Mar 19, 2018

Copy link
Copy Markdown
Contributor Author

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

guybedford force-pushed the main-skip-preserve-symlinks branch from 292c7bd to a9dc4f8 Compare March 20, 2018 14:42

guybedford commented Mar 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Mar 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

guybedford commented Mar 20, 2018

Copy link
Copy Markdown
Contributor Author

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

guybedford commented Mar 20, 2018

Copy link
Copy Markdown
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 mentioned this pull request Mar 28, 2018
Closed
guybedford force-pushed the main-skip-preserve-symlinks branch from 8991bcb to 994ed4e Compare March 31, 2018 12:48

guybedford commented Mar 31, 2018

Copy link
Copy Markdown
Contributor Author

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

Will actually merge this time.

benjamingr commented Apr 1, 2018

Copy link
Copy Markdown
Member

@guybedford forgot to merge :D ?

guybedford added a commit that referenced this pull request Apr 1, 2018
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 commented Apr 1, 2018

Copy link
Copy Markdown
Contributor Author

Merged in 141be92.

guybedford closed this Apr 1, 2018
targos pushed a commit that referenced this pull request Apr 2, 2018
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 mentioned this pull request Apr 4, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
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

Back | FazBrowse Home | New Git URL