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

breaking change: preserveSymlinks is false by default #135

Merged
merged 1 commit into from Apr 7, 2018

Conversation

@zkochan
Copy link
Contributor

@zkochan zkochan commented Sep 1, 2017

This is a follow-up to #131

followup to #131
Copy link
Member

@ljharb ljharb left a comment

Thanks!

I'm not sure there's any rush to release this breaking change; I'd love to keep this PR open though so that when one comes up, we can merge this!

@zkochan
Copy link
Contributor Author

@zkochan zkochan commented Sep 1, 2017

OK, what about a pre-major with a next dist-tag?

@ljharb
Copy link
Member

@ljharb ljharb commented Sep 1, 2017

I'm not clear on what the value would be, when we don't know what other major changes would later be included, and when we don't know when a major release might be (in a week? in a month? in a year?)

In general I'm also not a fan of prereleases; I think if it needs to be released, it should be a real release.

@zkochan
Copy link
Contributor Author

@zkochan zkochan commented Sep 1, 2017

OK, I don't insist.

My issue is that I created some PRs to pass preserveSymlinks: true to resolve, in packages that don't work with pnpm currently. Some maintainers don't respond though. So I am implementing a hook-system that will be able to override a sub-dependency.

What the hook does is basically overriding "resolve": "^1.4.0" with "resolve": "zkochan/node-resolve".

With these changes released as beta, I could rewrite resolve with "resolve": "^2.0.0-beta.0" instead.


So this is my reason. I am OK to wait, but I think it is unreasonable to wait for a whole year as this is how it should have worked to begin with.

@ljharb
Copy link
Member

@ljharb ljharb commented Sep 1, 2017

The maintainer would need to respond to include a breaking change anyways :-)

@zkochan
Copy link
Contributor Author

@zkochan zkochan commented Feb 12, 2018

@ljharb now that browserify doesn't preserve symlinks by default (from v16), maybe it is time to release resolve@2 with preserveSymlimks false by default?

@ljharb ljharb force-pushed the zkochan:master branch from dbfa8a8 to cdd4caf Apr 5, 2018
@ljharb
ljharb approved these changes Apr 5, 2018
@ljharb ljharb force-pushed the zkochan:master branch 2 times, most recently from 77517c3 to 75108bc Apr 5, 2018
@ljharb ljharb merged commit 75108bc into browserify:master Apr 7, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zkochan
Copy link
Contributor Author

@zkochan zkochan commented Jul 5, 2018

Is something blocking the release of resolve v2?

@ljharb
Copy link
Member

@ljharb ljharb commented Jul 6, 2018

Yes; I'm trying to include as many breaking changes into it as possible before releasing.

@zkochan
Copy link
Contributor Author

@zkochan zkochan commented Sep 6, 2018

An entire year has passed. Releasing this would mean a lot to pnpm.

@ljharb
Copy link
Member

@ljharb ljharb commented Sep 6, 2018

I'm confused, why does the default matter to pnpm? All the current libs using qs v1 might not upgrade, or might upgrade while setting the option explicitly to true.

@zkochan
Copy link
Contributor Author

@zkochan zkochan commented Sep 6, 2018

But there will be still a big chunk that will update and will not change preserveSymlinks. Which would be a big improvement already.

ljharb added a commit that referenced this pull request Dec 23, 2019
  - [Breaking] make `preserveSymlinks` false by default (#135)
  - [Breaking] `packageFilter`: bring sync/async into alignment; pass three args: pkg, pkgfile, dir (#171)
  - [Breaking] add `isDirectory`; error early if provided `basedir` is not a directory (#154)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants