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

Add package-lock.json #4337

Closed
wants to merge 1 commit into from
Closed

Conversation

benmccann
Copy link
Contributor

When using npm 5 to build a package-lock.json file is created with the following message:

npm notice created a lockfile as package-lock.json. You should commit this file

@etimberg etimberg added this to the Version 2.7 milestone Jun 6, 2017
@etimberg
Copy link
Member

etimberg commented Jun 6, 2017

Looks good to me. @simonbrunel ?

@simonbrunel
Copy link
Member

simonbrunel commented Jun 7, 2017

If I understand correctly, the package-lock.json only makes sense if:

  • Travis is setup to build on Node.js 8 / npm 5 (not sure if it's supported yet),
  • all contributors switch their environment to Node.js 8 (6.11.0 LTS is currently recommended),
  • package-lock.json is not updated in every PR (what npm commands update this file?)

If we can't get Travis to use this package-lock.json file, then I would not add it to the repository because builds will not reflect the locked dependencies. And since most of the dependencies are dev, does it make really sense to lock these deps? I would rather use pinned versions for the 2 non dev dependencies we have (moment and chartjs-color) and git ignore this package-lock.json.

Don't you think that this file will finally generate more noise and work than benefits?

@benmccann
Copy link
Contributor Author

package-lock.json doesn't require a certain version of Node. Only of npm

I use node 6 to build locally, which is the latest LTS as you've pointed out. I've sent a separate PR to update Travis to use node 6 instead of node 5

Travis uses npm 3 by default regardless of node version, so you're right that it's ignoring this file at the moment. For people who use npm 5 they're going to be continually bugged to check in this file. So it seems like we should either ask Travis to use npm 5 or maybe add this file to .gitignore?

@simonbrunel
Copy link
Member

simonbrunel commented Jun 7, 2017

I'm still thinking it will bring more noise than benefits, so my personal choice would be to .gitignore it.

I suggested Node.js 8 because I think npm 5 comes bundled in that version (at least on Windows), but you right, it has nothing to do with the version of Node.js.

edit: seems I removed the .gitignore suggestion in my previous comment before posting

@simonbrunel
Copy link
Member

@etimberg what do you think?

@etimberg
Copy link
Member

etimberg commented Jun 7, 2017

I think it really depends on how often we update it. It's noisy if it's always updating

@benmccann
Copy link
Contributor Author

I believe it will only update upon request or when package.json is updated.

However, I'm going to close this for now because I discovered there's a bug in npm's handling of this file that's a dealbreaker. I may reopen when the bug is fixed

@benmccann benmccann closed this Jun 8, 2017
@benmccann benmccann mentioned this pull request Jul 11, 2017
@benmccann benmccann deleted the package-lock branch August 1, 2017 04:04
@simonbrunel simonbrunel removed this from the Version 2.7 milestone Aug 28, 2017
@benmccann benmccann mentioned this pull request Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants