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

Remove yarn.lock from repo #33

Closed
viankakrisna opened this issue Jul 7, 2017 · 13 comments
Closed

Remove yarn.lock from repo #33

viankakrisna opened this issue Jul 7, 2017 · 13 comments

Comments

@viankakrisna
Copy link
Contributor

Relevant comment
facebook/create-react-app#2014 (comment)

Possible solution
facebook/create-react-app#2700

@sudo-suhas
Copy link
Collaborator

sudo-suhas commented Jul 7, 2017

Relevant to the discussion, I found this article quite helpful - http://jpospisil.com/2017/06/02/understanding-lock-files-in-npm-5.html

If you’re working on a library (as in a package onto which others will depend on), you should use the new lock file. An alternative is to use shrinkwrap but make sure it never gets published with the package (the new lock file is never published automatically)....

@viankakrisna
Copy link
Contributor Author

@sudo-suhas great read! but not conclusive... It just tell the pros and cons of having a lock file.

Unfortunately, you might not be able to notice that until a bug report comes in. Without any lock files in the repository, your build would fail at least on the CI because it would always install the latest versions of the dependencies and thus run the tests with the new broken version (provided that the build is run periodically, not just for PRs). With the lock in place however, it will always install the working locked version.

There’s a couple of solutions to this problem however. First, you could sacrifice the exact reproducibility and not add the lock file to your version control system. Second, you could make a separate build configuration which would run npm update prior running the tests. Third, you simply delete the lock before running the tests in the special build. How to actually deal with the broken dependency once discovered is another topic on its own mainly because semver as implemented by NPM doesn’t have a concept of allowing a wide range but also blacklisting specific versions.

This of course begs the question whether it’s actually worth it to add the lock file into the version control when working on libraries. A thing to keep in mind however is that the lock file contains not only dependencies but also dev dependencies. In that sense working on a library is similar to working on an application (see the next section) and having the exact same dev dependencies over time and across multiple machines is an advantage.

So is it a yes to remove it on pre-commit?

@sudo-suhas
Copy link
Collaborator

It just tell the pros and cons of having a lock file.

That's one of the reasons I liked it better.

So is it a yes to remove it on pre-commit?

Not sure. Maybe solve problems when you start facing them? Nothing wrong with having an iterative process.

@sudo-suhas
Copy link
Collaborator

Just realized, dunno if you noticed, but because of the files entry in package.json, yarn.lock will not be part of the package for the end user.

@asfktz
Copy link
Owner

asfktz commented Jul 7, 2017

package.json will be, but not yarn.lock and package-lock.json

https://docs.npmjs.com/files/package.json#files

Certain files are always included, regardless of settings:

package.json
README
CHANGES / CHANGELOG / HISTORY
LICENSE / LICENCE
NOTICE
The file in the "main" field
README, CHANGES, LICENSE & NOTICE can have any case and extension.

Conversely, some files are always ignored:

.git
CVS
.svn
.hg
.lock-wscript
.wafpickle-N
..swp
.DS_Store
._

npm-debug.log
.npmrc
node_modules
config.gypi
*.orig
package-lock.json (use shrinkwrap instead)

@viankakrisna
Copy link
Contributor Author

Yeah, but the issue is how to catch errors early when we develop the plugin. It's what CRA is doing, so, I just think it's nice if we follow it :)

@asfktz
Copy link
Owner

asfktz commented Jul 9, 2017

To be honest, I just not getting it.

We don't want to have yarn.lock in the repo?
If so, why no just .gitignore it?

@sudo-suhas
Copy link
Collaborator

sudo-suhas commented Jul 9, 2017

So here is what I understand:

  • Having a lock file(yarn.lock/package-lock.json) ensures that the dependency tree is deterministic. Meaning that if you have a dependency tree and it works, even if someone else was to run yarn install at a later point in time, the dependency tree inside node_modules would be exactly the same. While some underlying libraries could have newer versions which are legal according to your package.json specified version("^1.2.3" matches "1.2.4" and "1.3.0" etc.), it does not affect you.
  • So the downside to this is that if all libraries locked their dependency versions, package managers wouldn't be able to dedupe dependencies. Let's say you depended on packages a and b. If both a, b dependended on c like so:
    // package.json for `a`
    {
      // ...
      "dependencies": {
        "c": "^1.2.3"
      }
    }
      // package.json for `b`
    {
      // ...
      "dependencies": {
        "c": "^1.3.0"
      }
    }
    In this situation, if there was no lockfile, v1.3.0 would satisfy both.
  • The current implementation of package-lock.json tries to leverage both factors. The lock file ensures deterministic installs for the people working on the library but this lock file is not shipped to the user. So dedupe works. But this has the downside of untested dependency trees. The author/maintainer of the library will not be testing the repo with the latest package dependency graph. This is what is happening for this repo as well.

According to Kat Marchán, one of the maintainers of npm, the current implementation of package-lock.json is the best of both worlds.

@asfktz
Copy link
Owner

asfktz commented Jul 9, 2017

Thanks for the in-depth explanation @sudo-suhas, I understands now.

I removed the file.

Thanks for the heads up, @viankakrisna

@asfktz asfktz closed this as completed Jul 9, 2017
@viankakrisna
Copy link
Contributor Author

It's nice if we have a pre-commit hook that removes the file so no one can accidentally commit it. :)

@asfktz
Copy link
Owner

asfktz commented Jul 9, 2017

It's nice if we have a pre-commit hook that removes the file so no one can accidentally commit it. :)

I'm in

@sudo-suhas
Copy link
Collaborator

How is a pre-commit hook better/different than just adding it to the git ignore file?

@viankakrisna
Copy link
Contributor Author

gitignore makes you not aware whether or not you have the file in the directrory. Not ignoring it means that that it would show up like a sore thumb when you run git status.

See the linked thread on the first comment, it's a pr specifically ignoring yarn.lock. A pre commit hook is just another safety net to avoid accidentally commiting the file

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

3 participants