Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

deep reify local links #44

Merged
merged 4 commits into from
Feb 7, 2020
Merged

deep reify local links #44

merged 4 commits into from
Feb 7, 2020

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Feb 5, 2020

Handle link deps properly in ideal/virtual/reify

Fix: #43

This fixes the bug identified by npm/cli#750

Link deps that point to a location underneath the root package are fully
resolved and reified. Links external to the root path are left alone,
because they exist in a different project and presumably have their own
dep resolution being done separately.

Also, Link nodes now have their resolved field properly relativized to the directory where the link resides, rather than against the link path itself, since that is how symlinks work.

cc: @wesleytodd I believe this may have some impacts on your explorations of workspaces. Not sure if it's helpful or harmful, but most likely something to be aware of.

Fix: #43

This fixes the bug identified by npm/cli#750

Link deps that point to a location underneath the root package are fully
resolved and reified.  Links external to the root path are left alone,
because they exist in a different project and presumably have their own
dep resolution being done separately.
Should be the relative path from the link's containing folder, not from
the link path itself.  Ie, it should be the actual argument passed to
fs.symlink() if one were to try to reify the Link node.

This changes a lot of snapshots, but the only one that's really relevant
is the handling in the YarnLock class, since all the other instances of
using consistentResolve to re-relativize a path were handling Link nodes
specially anyway (eg, in reify, shrinkwrap, etc.)
The reify() method now returns the *actual* actualTree, so even though
engine-mismatching optional deps remain in the package-lock, they're not
being passed in the resolved tree.

Filter the offending darwin-only module out of the snapshots so that Travis
(which is Linux) doesn't fail tests.  In that case, just manually verify that
fsevents is actually in the package-lock, even though it isn't in the snapshot.
@isaacs isaacs force-pushed the isaacs/deep-reify-local-links branch from dcb698a to eb03ee9 Compare February 5, 2020 03:02
@wesleytodd
Copy link
Contributor

wesleytodd commented Feb 5, 2020

cc: @wesleytodd I believe this may have some impacts on your explorations of workspaces. Not sure if it's helpful or harmful, but most likely something to be aware of.

I was directly doing this myself. So this is an improvement IMO. The one thing I would like to see is an option to also reify the external links. Your presumption is only valid because that is how other tools have treated it, but as I mentioned to @ruyadorno, that is because they were hacking on top of the filesystem based resolution. If you have something like import maps or yarn style PnP you don't have this requirement anymore. I think there are many use cases for having a one step process to reify across many unrelated "projects" if you need to work on them together.

@isaacs
Copy link
Contributor Author

isaacs commented Feb 5, 2020

@wesleytodd Thanks. I can certainly imagine cases where it would be unwelcome (or just impossible), and many others where it would be super convenient. (For myself, most of the times that I didn't want link deps messed with, it was because npm did the wrong thing, but Arborist doesn't, so it's usually going to be fine?)

In any event, it's just a boolean check at one point, so it's easy enough to add a flag for. I guess then the question would be whether this should be opt-in or opt-out. Any preference?

@wesleytodd
Copy link
Contributor

it was because npm did the wrong thing, but Arborist doesn't, so it's usually going to be fine?

Yeah that logic makes sense, but I am a big fan of it "doing the right thing!

I guess then the question would be whether this should be opt-in or opt-out. Any preference?

Opt in I think. My guess is that it will take time and usage for folks to see how this all works, and it is a bigger change if it is the default behavior. My use case is such that I am happy to opt-in, and I imagine others who dig in like me will feel the same.

@isaacs isaacs merged commit eb03ee9 into master Feb 7, 2020
@wraithgar wraithgar deleted the isaacs/deep-reify-local-links branch April 22, 2021 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] bad behavior in npm/cli#750 test case
2 participants