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

Checking in the yarn.lock file #1067

Closed
mcwhittemore opened this issue Oct 14, 2016 · 18 comments
Closed

Checking in the yarn.lock file #1067

mcwhittemore opened this issue Oct 14, 2016 · 18 comments

Comments

@mcwhittemore
Copy link

The current version controls docs say that the lock file should be checked in.

But looking at yarn's own lock file it looks like it was only checked in recently.

I work on a team that is trying to decide if we should be checking in our lock file. While the docs make it clear that this is what is expected, the docs of other package managers make it clear that this should not happen and give good explanation for why it should not.

I've tried to find a discussion about why yarn decided to act differently than these other managers, but it doesn't seem to be laying around.

Is this still something in flux?
If not, is there a place where we can learn why yarn has made this decision?

@jfirebaugh
Copy link
Contributor

As detailed in the links @mcwhittemore posted, the conventional wisdom for Bundler and Cargo is to check in the lockfile in applications, but not check in the lockfile for libraries. The links go into detail about the rationale, and it would seem that similar considerations apply to yarn. Is there a reason not to make similar recommendations, i.e. update the documentation to distinguish between applications and libraries?

@sheerun
Copy link
Contributor

sheerun commented Oct 14, 2016

libraries shoudn't use yarn.lock if they expect their dependencies to be installed separately

yarn can benefit from yarn.lock because it packages all of its dependencies within release (i'm guessing that's happening for .rpm .dev and other builds. it doesn't do so for npm build, but I think it should do so (just like bower).

So the answer is:

  1. Always use yarn.lock in your applications
  2. Use yarn.lock in your libraries only if you package your dependencies along your library

@develar
Copy link

develar commented Oct 15, 2016

@mourner @sheerun

Use yarn.lock in your libraries only if

contradicts to

Fully deterministic environment — node_modules will be identical across all GL JS developers' machines, Circle CI runs and release builds (mapbox/mapbox-gl-js#3364 (comment))

"Fully deterministic environment" only if yarn.lock checked in. Right? So, even libraries should check in yarn.lock file.

@palmerj3
Copy link

@develar only applications require a fully deterministic environment. Libraries should not lock in specific module versions unless it's absolutely required. If all libs bundled their lockfile then no one would get patch updates and it would significantly effect the entire node ecosystem.

@Pauan
Copy link

Pauan commented Oct 15, 2016

@palmerj3 That would be true if Yarn respected yarn.lock in dependencies, but according to James Kyle, Christoph Pojer, and @wycats it doesn't.

If I'm understanding correctly, that means libraries can safely check in yarn.lock, because it will be ignored: #838 (comment)

Whether libraries should check in yarn.lock is a separate question, but at least they won't destroy the ecosystem if they choose to do so.

@mcwhittemore
Copy link
Author

It sounds like a problem with checking in your yarn.lock file as a library author is that it means developers will always be running against the same set of modules versions even though upstream users might be (will be even?) getting later versions of some of your dependencies.

Assuming all of your dependencies follow semvar very well this is ok, but the moment a bad release lets sip an accidental breaking change new users will find out about it but maybe not you as your ci tests won't be plagued downstream change.

@ide
Copy link
Contributor

ide commented Oct 16, 2016

The problem with not checking in your yarn.lock file is that CI failures will be attributed to innocent commits. Ex:

  1. Your repo is at commit 1. You have a dependency at version 1.0.0. Your tests pass.
  2. The author of the dependency publishes 1.1.0 and introduces a bug or breaks semver.
  3. A few days later you push commit 2. CI picks up 1.1.0 of your dependency and your tests fail.

At a glance, the blame is attributed to commit 2 even though it's bug-free. CI also discovers the bug a few days after the problem -- wouldn't it be nicer to discover the problematic dependency closer to when it was published?

win-win

One solution to this is a service like Greenkeeper that updates your dependencies in your package.json when new versions are published. Greenkeeper would need to update yarn.lock as well, but the timeline would look like this:

  1. Your repo is at commit 1. You have a dependency at version 1.0.0. Your tests pass.
  2. The author of the dependency publishes 1.1.0 and introduces a bug or breaks semver.
  3. Greenkeeper notices the new version and automatically creates a commit that simulates running yarn add dependency@1.1.0. CI runs your tests and they fail, notifying you.
  4. You look into the error and see that it's the dependencies fault. You tell the author and in the meantime pin the dependency's version in your package.json (^1.0.0 -> 1.0.0). You commit this (let's name this commit 1.pin) and CI passes.
  5. A few days later you push commit 2. Your tests pass.

In this scenario, you've correctly attributed failing tests to their root causes (which you get by committing yarn.lock) and quickly discovered an accidental breaking change and defended against it (which you get by omitting yarn.lock). Sort of a "best of both worlds" outcome. Plus yarn install in your CI tests runs faster because yarn.lock speeds things up (https://yarnpkg.com/en/compare).

@Pauan
Copy link

Pauan commented Oct 16, 2016

@develar The mapbox-gl-js library precompiles into a single dist/mapbox-gl.js file. That qualifies as "Use yarn.lock in your libraries only if you package your dependencies along your library", therefore it should use a yarn.lock file.

But that doesn't mean every library should use yarn.lock. It depends on how the library is distributed, whether it is precompiled or not, whether you are using something like Greenkeeper or not, etc.

These are the facts (some of which have already been pointed out by @sheerun, @mcwhittemore, and @ide):

  • yarn.lock is ignored in dependencies (i.e. libraries)

  • Therefore, if a library uses yarn.lock its unit tests will pass (because the unit tests are using yarn.lock), but consumers of the library might be broken (because the yarn.lock file is ignored and a sub-dependency breaks).

  • If a library bundles its dependencies (e.g. by precompiling), then that means consumers of the library will not break based upon the library's sub-dependencies. Therefore, it's okay to use yarn.lock

  • If a library or application uses yarn.lock it should regularly use yarn upgrade, and it should consider using automatic upgrades (e.g. Greenkeeper).

  • Regardless of whether a library uses a yarn.lock file or not, unit tests should be run regularly, in order to catch any breaking changes in dependencies.

  • When running unit tests, it is useful to first run the tests without changing any code, because if your tests fail, you know that it is because of dependencies and not because of any code changes.

    After you have verified that your code works with the latest versions, you can then run the tests with your code changes.

@jholster
Copy link

jholster commented Nov 4, 2016

yarn.lock is poor choice for file name, easily mistaken for unix lock files (used to "lock" a directory for specific process). Caused me lot of frustration before I realized that these are not lock files and you are actually supposed to check them in.

@mcwhittemore
Copy link
Author

I think this question has been pretty well answered so I'm going to close this. Other convos about lock files should probably be in their own issues.

@Pauan
Copy link

Pauan commented Nov 7, 2016

@mcwhittemore I disagree. Yes the question has been answered in this issue, but the Yarn documentation has not been changed. That means that anybody else who has the same question has to ask it again.

I think it would be useful for the Yarn docs to clarify how libraries should handle yarn.lock, even if that clarification is "always check in yarn.lock, even in libraries"

@wyze
Copy link
Member

wyze commented Nov 7, 2016

@Pauan, that issue should be opened on yarnpkg/website to add documentation.

@Pauan
Copy link

Pauan commented Nov 8, 2016

@wyze That makes sense, thanks for the link. 👍

@GeoffreyPlitt
Copy link

Shouldn't the comment THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. be removed from the yarn.lock file? We should encourage engineers to be comfortable editing it in the event of branch/merge conflicts, and treat it like a source file (OK to edit) instead of an auto-generated file (never edit).

@sheerun
Copy link
Contributor

sheerun commented Dec 9, 2016 via email

@GeoffreyPlitt
Copy link

I'm talking about when 2 branches made changes to it and you have to merge and handle conflicts. Requires manual editing.

@ljharb
Copy link

ljharb commented Dec 9, 2016

In those cases, the second person should rebase on top of the firsts merged changes, and use yarn to manually recreate their commit - not manually editing to fix the conflicts.

@GeoffreyPlitt
Copy link

Gotcha, 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

No branches or pull requests