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

Update knit proposal #73

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions accepted/0000-yarn-knit.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ This is a proposal to improve upon `yarn link` so that developers can more accur

# Motivation

`yarn link` (and `npm link`) before it have several problems when working on code bases of non-trivial sizes, especially with multiple apps. The current `link` command doesn't isolate `node_modules` between apps (especially problematic with the advent of Electron), it doesn't allow for working on multiple versions of a library, and it produces a `node_modules` hierarchy that is not faithful to the one produced after the library is published.
`yarn link` (and `npm link` before it) have several problems when working on code bases of non-trivial sizes, especially with multiple apps. The current `link` command doesn't isolate `node_modules` between apps (especially problematic with the advent of Electron), it doesn't allow for working on multiple versions of a library, and it produces a `node_modules` hierarchy that is not faithful to the one produced after the library is published.

# Detailed design

Expand Down Expand Up @@ -52,6 +52,24 @@ This behaves like `yarn add dep` except that it looks at the versions of `dep` t

`yarn knit dep` then runs most of the same installation steps that `yarn add dep` would. It creates `app/node_modules/dep` and creates symlinks for each of the symlinks under `~/.yarn-knit/dep-X.Y.Z`. Then it installs the dependencies of `dep` as usual by fetching them from npm. Finally it runs postinstall scripts.

### Running "yarn install --knit" inside of `app`

This behaves like normal `yarn install` but automatically links any dependencies that have been set up locally with `yarn knit`. This is very useful when you have an application with many locally developed dependencies so that you don't have to manually run `yarn link dep` for each one inside your app.

# Comparison with other Yarn features

There are already several ways of linking dependencies in Yarn. This section is a comparison of them, and explains why `yarn knit` is still valuable to add.

* `yarn link` is similar to `yarn knit` but links directories instead of files. This means that any `node_modules` inside linked dependencies are also included, breaking the flattened and deduped tree that yarn normally provides. Many other differences have already been discussed in this RFC. Ideally, the behavior of `yarn link` would be what is proposed in this RFC, but for backwards compatibility and to match the behavior of `npm link`, `yarn link` would stay around as is.

* `link://` dependencies are similar to `yarn link` but actually closer to what `yarn knit` would provide. `link://` dependencies also cause the linked module's dependencies to be installed locally, so as long as there is no `node_modules` folder inside the linked dependency, it would work identically to `yarn knit dep`. The existing implementation of `link://` dependencies could be used to implement `yarn knit dep`.

* `file://` dependencies cause files to be copied instead of linked. This is not as useful because it means you must re-install every time a change is made to the dependency.

* `yarn pack` + `yarn install dep.tgz` is similar to `file://` dependencies. The pack + install process must be re-run every time a change is made. It does correctly dedupe dependencies, however, as `node_modules` are excluded by `yarn pack`.

* [Workspaces](https://github.com/yarnpkg/rfcs/pull/66) are similar but solve a different problem. Where workspaces are great for a tree of related modules (e.g. a monorepo), `yarn knit` is for linking together modules in separate trees, e.g. things that might be shared between multiple workspaces.

Copy link
Member

Choose a reason for hiding this comment

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

Nice overview.

# How We Teach This

This proposal is mostly additive and affects only how people work on libraries that they are using in their apps. We would want to document the `knit` command in the "CLI Commands" section of the docs and perhaps add a new section to "The Yarn Workflow".
Expand All @@ -60,18 +78,16 @@ This proposal is mostly additive and affects only how people work on libraries t

# Drawbacks

One issue with this proposal is that it's not clear what to put in the lockfile after running `yarn link dep` since we don't have an npm URL for the dep yet -- it hasn't been published to npm.
One issue with this proposal is that it's not clear what to put in the lockfile after running `yarn knit dep` since we don't have an npm URL for the dep yet -- it hasn't been published to npm.

Another issue is that if you change package.json in `dep`, namely changing a dependency or modifying the `files` entry, you have to run `cd dep; yarn knit; cd app; yarn knit dep`.
Another issue is that if you change package.json in `dep`, namely changing a dependency or modifying the `files` entry, you have to run `cd dep; yarn knit; cd app; yarn knit dep`. Same if you add a file at the top level of the dependency since each file is linked individually. This isn't so bad as those changes are probably more rare than saving changes to existing files.

Also, if you update the code in dep and bump its version, say from 1.0.0 to 1.1.0, the symlinks in ~/.yarn-knit/dep-1.0.0 will still point to the code in your working directory, which now contains 1.1.0 code.

The symlinks might break but I think that's mostly OK since at that point you're done working on dep and have published it to npm and it's easy to go run yarn add dep in app and not use the symlinks anymore.
The symlinks might break but I think that's mostly OK since at that point you're done working on dep and have published it to npm and it's easy to go run `yarn add dep` in app and not use the symlinks anymore.

If you want to truly pin the versions of knitted packages then you'd need to have a different working directory for each version. (Git worktrees are great for this use case actually. Worktrees let you check out a repo once and then magically create semi-clones of it in separate directories, with the constraint that the worktrees need to be on different branches, which is totally OK in this scenario. The worktrees all share the same Git repo though, so if you commit in one worktree you can cherry pick that commit within another worktree.)

# Alternatives

Another similar approach is to run `yarn pack`, which creates a tarball of the library's contents as if it were downloaded from npm, and install the tarball in the app used to test the library. This has the benefits of reducing divergence between development and production -- `library/node_modules` is not shared across apps, the developer can work on multiple versions of the library, and the `node_modules` hierarchy is faithfully represented. The downside is that everytime the developer edits a file, they need to re-pack and reinstall the library.
Finally, another issue is with the way the node require resolution algorithm works. Dependencies of symlinked modules are resolved relative to the realpath, not the symlink. This means that you'll still get duplicates if both modules depend on a third dependency, or errors if that dependency is not installed in either place. This is solved by the recently added runtime option for node `--preserve-symlinks`, which skips getting the realpath when resolving modules. Something similar would need to be added to browserify/webpack to solve this there as well. I recently opened a [PR for browserify](https://github.com/substack/node-browserify/pull/1742) to support the same option.
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is a problem with knitting because on install the knitted dependencies should not have subdependencies packed in the "repository".
So all subdependencies should be installed fresh at referer level.

Copy link
Author

Choose a reason for hiding this comment

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

The issue is that the linked module may still have a node_modules folder where you are developing. When you require('dep') from your app, it gets resolved using the realpath, e.g. ~/dev/dep/index.js. If that file has require('dep2'), it is resolved to ~/dev/dep/node_modules/dep2, and that copy will be used instead of the one installed in your app. I verified this to be the case. It is not the case with the --preserve-symlinks option, because the resolution algorithm uses the symlink path (~/dev/app/node_modules/dep/index.js) instead of the realpath, so when resolving dep2, ~/dev/app/node_modules/dep2 will be used instead.

Copy link
Member

Choose a reason for hiding this comment

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

Does knitting mean that dependencies need run prepublish script and the files listed in package.json/files should be copied to some temp location?

I think there would not be a symlink ~/dev/app -> ~/dev/dep, it would probably be ~/dev/app -> (yarn-cache)/dep-knit-<uuid> and node_modules would be empty there.

Copy link
Author

Choose a reason for hiding this comment

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

Copying files kinda defeats the purpose, no? Then you'd have to run yarn knit every time you make a change to a dependency. I think the symlink would be ~/dev/app/index.js -> ~/.yarn-knit/dep/index.js -> ~/dev/dep/index.js for example. The realpath is still the last one, which may have a node_modules folder in it.

Copy link
Member

@bestander bestander Jul 11, 2017

Choose a reason for hiding this comment

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

Then there is a contradiction we need to sort out:

https://github.com/devongovett/rfcs/blob/50712e27d2d242bf466224186fcbb87b9bbab0ba/accepted/0000-yarn-knit.md#isolating-node_modules-correctly

https://github.com/devongovett/rfcs/blob/50712e27d2d242bf466224186fcbb87b9bbab0ba/accepted/0000-yarn-knit.md#faithfully-representing-the-node_modules-hierarchy

https://github.com/devongovett/rfcs/blob/50712e27d2d242bf466224186fcbb87b9bbab0ba/accepted/0000-yarn-knit.md#a-practical-proposal----knitting

The links above indicate that knitting would simulate full npm publishing/installation but without actual publish/install to npm registry.
Symlinking app -> dep breaks this simulation because dep will have non publishable artifacts (node_modules, .yarnignore files etc)

Copy link
Author

Choose a reason for hiding this comment

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

That's what the middle symlink is for. Only the published files would have a symlink in ~/.yarn-knit/dep, so (as long as --preserve-symlinks is used) non-published files would not show up to the app.

Copy link
Member

Choose a reason for hiding this comment

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

I see.
Then node_modules would not be symlinked and this is not a problem.


# Unresolved questions