GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — module: add --preserve-symlinks command line flag by jasnell · Pull Request #6537 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented May 2, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

module

Description of change

#5950 sought to address a bug in the module loader with regards to symlinked peer dependencies. The PR passed CITGM and CI on multiple runs and was landed as a semver-major in v6. Unfortunately, it came to light after that the change breaks a number of other cases.

This reverts the change and adds two additional test cases (one for the original behavior, and a known-issue test for the symlinked peer dependency case.
This puts the new functionality behind the --preserve-symlinks command line flag. The pre-v6 behavior is the default. Using the --preserve-symlinks command line option will make Node.js use the new behavior.

This should be considered to be a temporary solution until a better fix for the symlinked peer dependencies problem is figured out.

Refs: #5950

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
alexanderGugel, fengmk2, hacke2, DavidCai1111, saper, mantoni, STRML, jamestalmage, nowtvicebergteam, dead-horse, and 2 more reacted with thumbs up emoji warren-lux and arcanis reacted with confused emoji warren-lux, nowtvicebergteam, and funtrix reacted with heart emoji
All reactions
  • 👍 12 reactions
  • 😕 2 reactions
  • ❤️ 3 reactions
@jasnell jasnell added the module Issues and PRs related to the module subsystem. label May 2, 2016
@MylesBorins

MylesBorins commented May 2, 2016

Copy link
Copy Markdown
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/2469/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 2, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd ... may fail on windows... I added another commit that adds gates for windows symlink permissions
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced May 2, 2016
Revert "module: preserve symlinks when requiring" #6536
Closed
Possible Windows Node 6 regression with spawn and a substed drive #6500
Closed
@Trott

Trott commented May 2, 2016

Copy link
Copy Markdown
Member

Is reverting a semver-major commit itself a semver-major change?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 3, 2016

Copy link
Copy Markdown
Member Author

No, this would be a bug fix.
alexanderGugel and jamestalmage reacted with thumbs up emoji alexanderGugel reacted with heart emoji
All reactions
  • 👍 2 reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dlongley

dlongley commented May 3, 2016

Copy link
Copy Markdown

Can we, instead, just do realpath on binary modules?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 3, 2016

Copy link
Copy Markdown
Member Author

@dlongley ... At this point I definitely think the safest thing to do would be to revert for now then keep working on the problem. The one thing that is painfully clear is that our existing test cases do not have sufficient coverage of functionality that users have come to depend on. Reverting this for now does not mean we're giving up on it, it just means we take a step back, get the test coverage built up that we need to, and take another run at the problem.
dlongley, hax, alexanderGugel, jamestalmage, and intentionally-left-nil reacted with thumbs up emoji alexanderGugel and intentionally-left-nil reacted with hooray emoji
All reactions
  • 👍 5 reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed May 3, 2016
View reviewed changes
Comment thread test/addons/symlinked-module/submodule.js Outdated

@bnoordhuis bnoordhuis May 3, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

assert.strictEqual?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell May 3, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yeah, there are several issues I was noticing with this commit. I modified it just enough to get it running but hadn't gone through a detailed review yet.

@saper .. If you don't mind I'm going to go through and clean these up. I'll keep your name on the commit tho.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@hax hax mentioned this pull request May 3, 2016
node 6 下 npminstall 的 cache 不行了 cnpm/npminstall#73
Closed
@alexanderGugel alexanderGugel mentioned this pull request May 3, 2016
module: resolve _-prefixed dependencies to real path #6460
Closed
3 of 4 tasks
@Fishrock123

Fishrock123 commented May 3, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This sounds major to me.

No, this would be a bug fix.

Can you explain in detail more?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the revert-5950 branch from dae062c to 468096d Compare May 3, 2016 16:05
@jasnell

jasnell commented May 3, 2016

Copy link
Copy Markdown
Member Author

It's been a long conversation across multiple threads. This commit, while fixing a legitimate bug, broke several other aspects of module loading. Given that, it makes more sense to revert as a bug fix to resolve those issues and take another run at solving this problem in a different way.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123
Fishrock123 force-pushed the master branch from 05f4313 to eb12f93 Compare May 3, 2016 16:46
@saper

saper commented May 3, 2016

Copy link
Copy Markdown

@jasnell I have amended my commit (your branch is at saper@2500a92). No jshint complaints and two node engines tested - one with a bug and one without.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@saper

saper commented May 3, 2016

Copy link
Copy Markdown

Maybe it's just a tone of the commit messages but I am not sure why we praise #3402 so much here. This should just revert the change and explain why. The tests for #3402 should be introduced somewhere else (if at all).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 3, 2016

Copy link
Copy Markdown
Member Author

The commit message is written to acknowledge the fact that the intent of the reverted commit was to fix a bug that still needs to be fixed after the revert.I can add the new tests from the reverted PR to the known issues tests as part of this revert PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the ctc-agenda label May 4, 2016
@rvagg rvagg mentioned this pull request May 4, 2016
Node.js Foundation Core Technical Committee (CTC) Meeting 2016-05-04 #6567
Closed
@jasnell

jasnell commented May 4, 2016

Copy link
Copy Markdown
Member Author

CTC discussed this today and agreed to move forward with the revert in v6.x.

I will be working on putting the new behavior introduced by #5950 behind a command line flag.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell removed the ctc-agenda label May 4, 2016
@wldcordeiro

wldcordeiro commented May 4, 2016

Copy link
Copy Markdown

@jasnell If I understood you right having the behavior from pre-6.0 will be behind the flag?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 4, 2016

Copy link
Copy Markdown
Member Author

No, the pre-6.0 behavior will be the default, the symlinked peer dependency fix will be what is behind a flag.
nowtvicebergteam and fengmk2 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 12, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd ...updated to use common.skip()
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 13, 2016

Copy link
Copy Markdown
Member Author

CI is green... will land tomorrow if there are no objections from @nodejs/ctc or @nodejs/collaborators.
nowtvicebergteam reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 13, 2016

Copy link
Copy Markdown
Member Author

One final CI: https://ci.nodejs.org/job/node-test-pull-request/2635/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
src: add --preserve-symlinks command line flag
a954c36 
Add the `--preserve-symlinks` flag. This makes the changes added
in nodejs#5950 conditional. By default the old behavior is used. With
the flag set, symlinks are preserved, switching to the new
behavior. This should be considered to be a temporary solution
until we figure out how to solve the symlinked peer dependency
problem in a more general way that does not break everything
else.

Additional test cases are included.
@jasnell
jasnell force-pushed the revert-5950 branch from f224001 to a954c36 Compare May 13, 2016 18:42
jasnell added a commit that referenced this pull request May 13, 2016
@jasnell
src,module: add --preserve-symlinks command line flag
5d38d54 
Add the `--preserve-symlinks` flag. This makes the changes added
in #5950 conditional. By default the old behavior is used. With
the flag set, symlinks are preserved, switching to the new
behavior. This should be considered to be a temporary solution
until we figure out how to solve the symlinked peer dependency
problem in a more general way that does not break everything
else.

Additional test cases are included.

PR-URL: #6537
Reviewed-By: Ben Noordhuis <[email protected]>
@jasnell

jasnell commented May 13, 2016

Copy link
Copy Markdown
Member Author

Landed in 5d38d54
MylesBorins, fengmk2, and Tonimo reacted with hooray emoji
All reactions
  • 🎉 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this May 13, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
@jasnell @evanlucas
src,module: add --preserve-symlinks command line flag
95b7560 
Add the `--preserve-symlinks` flag. This makes the changes added
in #5950 conditional. By default the old behavior is used. With
the flag set, symlinks are preserved, switching to the new
behavior. This should be considered to be a temporary solution
until we figure out how to solve the symlinked peer dependency
problem in a more general way that does not break everything
else.

Additional test cases are included.

PR-URL: #6537
Reviewed-By: Ben Noordhuis <[email protected]>
@evanlucas evanlucas mentioned this pull request May 17, 2016
2016-05-17, Version 6.2.0 (Stable) #6810
Merged
evanlucas added a commit that referenced this pull request May 17, 2016
@evanlucas
2016-05-17, Version 6.2.0 (Stable)
a4705f6 
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)

As of this release the 6.X line now includes 64-bit binaries for Linux
on Power Systems running in big endian mode in addition to the existing
64-bit binaries for running in little endian mode.

PR-URL: #6810
evanlucas added a commit that referenced this pull request May 17, 2016
@evanlucas
2016-05-17, Version 6.2.0 (Stable)
26120e2 
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)

As of this release the 6.X line now includes 64-bit binaries for Linux
on Power Systems running in big endian mode in addition to the existing
64-bit binaries for running in little endian mode.

PR-URL: #6810
@dlongley dlongley mentioned this pull request May 23, 2016
npm link doesn't play nice with peer dependencies npm/npm#5875
Closed
@rosswarren rosswarren mentioned this pull request Jul 8, 2016
Add --preserve-symlinks flag for node mochajs/mocha#2364
Merged
This was referenced Sep 7, 2016
Not working on Node V6 (T7318) babel/babel#4261
Closed
Node 6.0 breaks babel-node ("Unexpected token import") (T7336) babel/babel#4275
Closed
@MylesBorins MylesBorins mentioned this pull request Jan 6, 2017
Potential Semver Minor Backports nodejs/Release#177
Closed
9 tasks done
@MylesBorins MylesBorins added dont-land-on-v4.x labels Jan 16, 2017
This was referenced Jan 21, 2018
Support npm link development workflow react/create-react-app#1107
Closed
Resolve symlinks using Node resolution mechanism react/create-react-app#3883
Open
Add support for --preserve-symlinks jestjs/jest#5356
Closed
Don't use app node_modules folder as a resolve fallback react/create-react-app#3884
Merged
@guybedford guybedford mentioned this pull request Mar 16, 2018
--preserve-symlinks should (optionally?) apply to the require.main module #19383
Closed
@mrmeku mrmeku mentioned this pull request Dec 20, 2019
Preserve symlinks by default when running node under Bazel bazel-contrib/rules_nodejs#1487
Closed
@dayongkr dayongkr mentioned this pull request Aug 11, 2026
Question about the preserveSymlinks: true default Swatinem/rollup-plugin-dts#405
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@jasnell @MylesBorins @Trott @dlongley @Fishrock123 @saper @wldcordeiro @nowtvicebergteam @jamestalmage @xzyfer @bnoordhuis @addaleax @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.