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

add preserveSymlinks option #131

Merged
merged 1 commit into from
Jul 26, 2017
Merged

add preserveSymlinks option #131

merged 1 commit into from
Jul 26, 2017

Conversation

zkochan
Copy link
Contributor

@zkochan zkochan commented Jul 25, 2017

Ref #130

@zkochan @ljharb
[New]: add preserveSymlinks option
fed3f0f 
Add the possibility to make `resolve` use real paths during resolution.

Fixes browserify#130.
@zkochan zkochan force-pushed the master branch 2 times, most recently from 991944d to 0fa967f Compare July 25, 2017 20:28
@zkochan
Copy link
Contributor Author

zkochan commented Jul 25, 2017

@ljharb could you please review this?

although Travis is failing on some node versions, it doesn't seem to be caused by these changes.

ljharb
ljharb approved these changes Jul 26, 2017
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great - tests should be fixed on master; after a rebase we should be good to go!

lib/node-modules-paths.js Outdated
@@ -11,6 +12,16 @@ module.exports = function nodeModulesPaths(start, opts) {
// resolving against the process' current working directory
var absoluteStart = path.resolve(start);

if (!opts || !opts.preserveSymlinks) {
try {
absoluteStart = fs.realpathSync(absoluteStart);
Copy link
Member

Choose a reason for hiding this comment

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

it is worth noting that this line means that this file will now not work in a browser; whereas i think it would have prior

test/symlinks.js
});

test('sync symlink', function (t) {
var start = new Date();
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to use Date.now() here, nbd tho

ljharb
ljharb reviewed Jul 26, 2017
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I've just done the rebase for you; but perhaps we shouldn't make this be a breaking change by enabling the behavior by default?

@zkochan
Copy link
Contributor Author

zkochan commented Jul 26, 2017 •
edited

Thanks!

but perhaps we shouldn't make this be a breaking change by enabling the behavior by default?

I think preserveSymlinks should be false by default because that is how Node resolution works by default. If you think this is dangerous or it is preferable to not make breaking changes (I know that this package is very widely used), I am fine to make it true now and then maybe in the future we'll make it false by default

@ljharb
Copy link
Member

ljharb commented Jul 26, 2017

Yeah - let's do that (have it default to true for now) and release a semver-minor.

I'll plan a breaking change in the future that flips it to false - if you'd like to make that PR after this one's merged, I'll rebase and merge it when it's time :-)

@zkochan
Copy link
Contributor Author

zkochan commented Jul 26, 2017

OK cool, I'll push the change in a few minutes

@zkochan zkochan force-pushed the master branch 2 times, most recently from 771468e to 3cd8620 Compare July 26, 2017 19:46
@zkochan
Copy link
Contributor Author

zkochan commented Jul 26, 2017 •
edited

Changed preserveSymlinks to true by default

Added a notice to README about changing the default value of preserveSymlinks in next major version. It will help me convince users of resolve use preserveSymlinks: false earlier.

@ljharb ljharb merged commit fed3f0f into browserify:master Jul 26, 2017
This was referenced Jul 27, 2017
Closed
Closed
@zkochan zkochan mentioned this pull request Sep 1, 2017
Merged
zkochan added a commit to zkochan/module-deps that referenced this pull request Sep 2, 2017
@zkochan
Don't preserve symlinks when resolving deps
cd317c3 
For context see: browserify/resolve#131

This fixes browserify for projects that use pnpm.
Related issue at pnpm: pnpm/pnpm#795
@zkochan zkochan mentioned this pull request Sep 2, 2017
Merged
@zkochan zkochan mentioned this pull request Sep 24, 2017
Closed
7 tasks
ljharb pushed a commit to zkochan/node-resolve that referenced this pull request Apr 5, 2018
@zkochan @ljharb
[breaking] make preserveSymlinks false by default
cdd4caf 
followup to browserify#131
ljharb pushed a commit to zkochan/node-resolve that referenced this pull request Apr 5, 2018
@zkochan @ljharb
[breaking] make preserveSymlinks false by default
77517c3 
followup to browserify#131
ljharb pushed a commit to zkochan/node-resolve that referenced this pull request Apr 7, 2018
@zkochan @ljharb
[breaking] make preserveSymlinks false by default
75108bc 
followup to browserify#131
@ljharb ljharb mentioned this pull request Jul 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@zkochan @ljharb