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

Fixed: do not preserve symlinks as per Node's default behaviour #300

Closed
wants to merge 3 commits into from

Conversation

thomheymann
Copy link

@thomheymann thomheymann commented Aug 23, 2017

There's a bug in the current version of PostCSS import plugin that occurs when using symlinks and causes invalid dependencies to be loaded in when using different versions of the same dependency within the same app.

The bug occurs as the default behaviour of the resolve module (used to resolve ids to filenames) is different to the default behaviour of Node.js's native require.resolve method.

I know this is probably a bit of an edge case as most people don't use symlinks but it causes huge issues in monorepos (e.g. lerna / yarn workspaces).

The fix is to disable the following option in the resolve module:

  • opts.preserveSymlinks - if true, doesn't resolve basedir to real path before resolving. This is the way Node resolves dependencies when executed with the --preserve-symlinks flag. Note: this property is currently true by default but it will be changed to false in the next major version because Node's resolution algorithm does not preserve symlinks by default.
    (https://www.npmjs.com/package/resolve)

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 23, 2017

This is a crazy edge-case, but I agree it's worth fixing. However, technically, this is a breaking change (semver-major). I'd like to release a few other things in a major release other than this.

@thomheymann
Copy link
Author

Yeh I agree - It should probably be a major version bump just to make sure no one's relying on the previous behaviour.

Why being precious about version numbers? :)

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 23, 2017

There's just other semver-major stuff I've been thinking of doing. I don't mind releasing new versions, but major versions require manual effort for everyone to upgrade, so I don't like to do them too crazily often.

@thomheymann
Copy link
Author

How about exposing it as an option and leaving the default as is? Happy to modify the PR.

@thomheymann
Copy link
Author

I've updated the pull request.

Hopefully this will work for you, keeping current behaviour as is but adding a fix for those who want Node's default dependency resolution behaviour.

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 24, 2017

Sorry for being slow about getting back. Could you revert this PR back to it's original state? I'm not a fan of having too many options. We'll just release a major for this.

@RyanZim RyanZim added this to the 11.0.0 milestone Aug 24, 2017
@thomheymann
Copy link
Author

No worries - all done!

@RyanZim RyanZim self-assigned this Aug 25, 2017
@RyanZim
Copy link
Collaborator

RyanZim commented Aug 25, 2017

Thanks, will merge and release as soon as I can.

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 15, 2017

Man, just noticed I left this hanging. Will set a reminder and try to get on this ASAP. Sorry about this.

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 16, 2017

Landed in 80cadec. Thanks!

@RyanZim RyanZim closed this Sep 16, 2017
@RyanZim
Copy link
Collaborator

RyanZim commented Sep 16, 2017

Publishing in v11 now 🎉

@thomheymann
Copy link
Author

Awesome - Thanks!!

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.

2 participants