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

:link dependencies #34

Merged
merged 2 commits into from
May 4, 2017
Merged

:link dependencies #34

merged 2 commits into from
May 4, 2017

Conversation

mgcrea
Copy link
Contributor

@mgcrea mgcrea commented Dec 4, 2016

Following #25, hope the formatting is good enough (was unsure if I had to hard word wrap or not, and if yes how to deal with md links).

# Unresolved questions

Not sure how we should handle actually publishing packages with such
dependencies, the existing behavior for `file:` types?

Choose a reason for hiding this comment

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

I think it could be safe to say that the link: prefix could only be used on private package (private:true in package.json). It will avoid a lot of difficulties and weird decision to manage all the publish/install case.

Copy link

Choose a reason for hiding this comment

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

💯 that would completely remove my concern about this change infecting the ecosystem.

@armandabric
Copy link

In your point of view, how dependencies of the linked package should be managed?

  • Is they should be installed in the "parent" repo, or be installed in the linked repository?
  • Should they be installed before the link or will be installed on the fly with the other dependencies?

@jamiebuilds
Copy link
Contributor

Is this going to be a necessary feature if we add workspaces by merging Lerna into Yarn?

@benlangfeld
Copy link

benlangfeld commented Dec 7, 2016

Maybe I'm just being dense, @thejameskyle, but Lerna appears to be concerned only with managing separate packages contained within a monorepo, where those modules are intended to be published independently without any dependencies between them, and I don't see how it would help with the use case where a monorepo contains many modules which are dependencies of each other. Am I missing something?

@jamiebuilds
Copy link
Contributor

Lerna very much is designed around having cross-dependencies. The lerna bootstrap command is for linking them all together.

@benlangfeld
Copy link

benlangfeld commented Dec 7, 2016

Ah, I'd completely misunderstood how Lerna works. Thanks for the pointer @thejameskyle.

For anyone curious about @thejameskyle's mention of merging the projects, this appears to come from yarnpkg/yarn#946 (comment).

@mgcrea
Copy link
Contributor Author

mgcrea commented Jan 5, 2017

Bumping this to see if there is a chance it might get reviewed/accepted or if I should just close this for low interest?

@jamiebuilds
Copy link
Contributor

The highest priority for Yarn is working through issues in existing functionality. If you want RFCs to be accepted sooner you can come help us out over here: https://github.com/yarnpkg/yarn/issues

@moimikey
Copy link

this... needs to be merged in. it's so essential for mono repo approaches!

@jamiebuilds
Copy link
Contributor

jamiebuilds commented Feb 21, 2017

So we're actually finalizing a spec (which I'll open up an RFC for) for what "Yarn Workspaces" will look like, which will add monorepo support to Yarn.

Once we have workspaces I don't see much of a use case for this RFC. I'm going to close it with the promise that we'll have something better.

@jamiebuilds
Copy link
Contributor

Actually we should add this, it has valid use cases even with workspaces and npm is going to add it: npm/npm#15900

@jamiebuilds jamiebuilds reopened this Mar 1, 2017
@jamiebuilds
Copy link
Contributor

Note that we need to align on behavior with npm. cc @iarna

@jamiebuilds jamiebuilds changed the title feat(rfc): add 0000-link-dependency-type.md :link dependency type Mar 1, 2017
@jamiebuilds jamiebuilds changed the title :link dependency type :link dependencies Mar 1, 2017
@iarna
Copy link

iarna commented Mar 1, 2017

This seems in alignment with what npm is proposing. If you all spot any differences please speak up. Our spec goes into rather more detail about how this feature would be expressed in npm, but I don't think any of that is relevant to interoperability between the two projects.

@ljharb
Copy link

ljharb commented Mar 1, 2017

Wouldn't the two projects need to agree on what goes in package.json, since it could end up published to the ecosystem?

@mgcrea
Copy link
Contributor Author

mgcrea commented Mar 1, 2017

Great to have this feature back into discussion! Turns out I just updated my fork yesterday to rebase the changes, here is an up-to-date commit: mgcrea/yarn@69f02c0

It's lacking test but will gladly add them back once we reach a PR-ready stage.

- Either add a new `link:` prefix that would just create symlinks and that's it
(regardless of destination's existence)

- Add an option flag `--link-file-dependencies` that would override default
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this from the RFC? Just add link:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iarna
Copy link

iarna commented Mar 1, 2017

@ljharb I think the only point of differences is treating existing local directory dependencies (file:/path/to/directory/) just like link: dependencies. That is, file:/path/to/directory/ becomes an alias for link:/path/to/directory/. But other than that I think our RFCs are already aligned. Do you see any other differences?

@iarna
Copy link

iarna commented Mar 1, 2017

The only caveat is that we're actually still debating even using link:. How do you all feel about just having file: create links and not introducing a new specifier? I'd like there to be some wider discussion around this…

@mgcrea
Copy link
Contributor Author

mgcrea commented Mar 1, 2017

I personally wouldn't mind with only file: doing symlinks, but I guess there might be other people relying on the current file: mechanism. Adding the explicit link: and deprecating file: first sounds a bit safer (with a warning), I'd still go for that.

@iarna
Copy link

iarna commented Mar 2, 2017

I've updated the npm RFC to reflect the npm cli team's current consensus on this, which is that this should be a change to file: behavior and not introduce a new specifier.

It's worth noting that neither of the versions of my proposal for npm has us keeping legacy file: specifier behavior. All I changed was ditching a duplicate specifier named link: that otherwise acts identically.

From npm's perspective this is a breaking change, which is why this is scheduled for npm@5. It's my suspicion that this change won't actually break anyone, but as we're changing the contract we'll bump the major.

@benlangfeld
Copy link

What is left to decide here in order for this RFC to be accepted?

@bestander
Copy link
Member

@mgcrea, do you want to update this RFC to be inline with npm@5?

@bestander
Copy link
Member

However maybe I don't have enough context here but it feels like a complication merging file: and link: into a single specifier.
They are doing quite different things

@iarna
Copy link

iarna commented Apr 6, 2017

Making file: specifiers create links is drawing a line in the sand and saying that file: specifiers as they were implemented in npm@2 were a mistake. They're not content addressable. (A problem for shrinkwraps and locks.) There's no way to know if what's in your node_modules is the same was what the specifier points at. (A problem for npm's model, which never trusts that your
node_modules matches its shrinkwrap. Yarn avoids by pretending that drift can't happen. Which is fine, so long as your users never, ever use other tools or do weird things.) TLDR: file: specifiers that copy are a mess.

I'm wrapping up spec tests for the npm implementation over at spec-local-specifiers which y'all are welcome to crib.

Edit: the spec document itself needs some minor revisions around flattening behavior and NODE_PRESERVE_SYMLINKS

@bestander
Copy link
Member

Ok, I am in to follow npm here

@benlangfeld
Copy link

benlangfeld commented Apr 11, 2017

@bestander / @thejameskyle What can I do to contribute to this RFC and the matching implementation being merged?

@bestander bestander requested a review from arcanis April 13, 2017 16:32
@bestander
Copy link
Member

@benlangfeld, thanks for offering help.
Just hang on a few days, @arcanis the new Yarn's core team member, is getting ramped up on this issue.
After this we will start resolving all those issues around managing multiple local packages.

@bestander
Copy link
Member

FYI, we are planning to have final look at this on Thursday this week.
Workspaces is getting in active phase and I think link: will be needed for that, we dragged a little bit until now to make sure that we don't have conflicting features in Yarn that do the same job.

@benlangfeld
Copy link

benlangfeld commented May 3, 2017

Thanks @bestander. At this point, I'm willing to pay a yarn subscription if it gets this merged; anything so my team can keep their hair.

If there's anything that we can do to help move this forward, don't hesitate to delegate :)

@arcanis arcanis merged commit ac8f706 into yarnpkg:master May 4, 2017
@arcanis
Copy link
Member

arcanis commented May 4, 2017

Hi! We read the proposal together with @bestander, and really like it. We think the feature described in this RFC might prove quite valuable, partly because of the use cases it contributes to solve, and partly because it might be a solid base for implementing more complex features later.

@mgcrea I saw that you opened a pull request with a prototype implementation, do you still want to champion with feature? If so, I'd be happy to reopen it so that we can discuss it in more details. Let me know :)

In the meantime, let's merge this document!

@mgcrea
Copy link
Contributor Author

mgcrea commented May 4, 2017

@arcanis I'd be happy to. I'm off work this week so I can have some spare time to work on it.

Would we directly go for the (major?) breaking change of the file: dependency type like npm?

In that case, I can rewrite the PR accordingly, might need some extra-eyes to make sure there is no dead-code floating around (used exclusively by the current file: handling).

@bestander
Copy link
Member

bestander commented May 4, 2017 via email

@mgcrea
Copy link
Contributor Author

mgcrea commented May 9, 2017

Ok, I've quickly rebased my previous non-breaking work to yarnpkg/yarn#3359 we can discuss implementation details there.

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.

9 participants