Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Lock npm versions #8283

Closed
NejcZdovc opened this issue Apr 13, 2017 · 17 comments
Closed

Lock npm versions #8283

NejcZdovc opened this issue Apr 13, 2017 · 17 comments

Comments

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Apr 13, 2017

Describe the issue you encountered:
Lock all npm packages to the specific version, so that we are all working on the same dependencies.

In this issue I am NOT updating anything to the mayor version, just settings values that we get now if we run npm i

We should update all dependencies with every release.
cc @bbondy @alexwykoff @bsclifton

@NejcZdovc NejcZdovc added this to the 0.14.3 milestone Apr 13, 2017
@NejcZdovc NejcZdovc self-assigned this Apr 13, 2017
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Apr 13, 2017
Resolves brave#8283

Auditors: @bsclifton

Test Plan:
- test passing
@luixxiul
Copy link
Contributor

We should update all dependencies with every release.

Let's consider using greenkeeper #1701

@NejcZdovc
Copy link
Contributor Author

@luixxiul I like it, let me try it out

@bsclifton
Copy link
Member

bsclifton commented Apr 13, 2017

an Alternative we can consider too: shrinkwrap

@diracdeltas put together PR #7795 which I didn't feel comfortable accepting (yet) because I'm unfamiliar with shrinkwrap / am not sure how the update process would look like

@NejcZdovc
Copy link
Contributor Author

@bsclifton personally I prefer automated things, so this greenkeeper will force us to be up to date 😃

@alexwykoff
Copy link
Contributor

Greenkeeper will only make sense after we have a consistently passing CI otherwise things will 'magically break' and you will be hunting for a long time to figure out why.

@diracdeltas
Copy link
Member

I think we should lock NPM versions, but I don't think we should be updating the lock to the latest version of every package with every commit/PR. We tried doing that for brave/sync (which has far fewer dependencies) and it soon became difficult to track down what bugs were due to a dependency update.

It might be ok updating the lockfile once per release, as long as there is enough time for testers to catch bugs that are due to dependency updates.

If we lock down versions with shrinkwrap or Yarn, I can re-enable Travis dependency caching.

@luixxiul
Copy link
Contributor

It might be ok updating the lockfile once per release, as long as there is enough time for testers to catch bugs that are due to dependency updates.

I'm thinking about the possibility of updating packages based on the release channels.

eg

  • Nightly/Alpha -> major.minor.maintenance
  • Beta -> minor.maintenance
  • Release -> only .maintenance

It could only cause chaos, though. Just my idea.

@luixxiul
Copy link
Contributor

otherwise we could update deprecated packages at least.

@bbondy bbondy removed this from the 0.15.1 milestone Apr 17, 2017
@bsclifton
Copy link
Member

@NejcZdovc what do you think about accepting #7795 in lieu of this PR?

@NejcZdovc
Copy link
Contributor Author

@bsclifton @diracdeltas correct me if I am wrong, but doesn't this only apply when travis or our build pipe is using npm. When I as developer clone browser-laptop and run npm i shrinkwrap is not used and we will have different versions of packages. I think that we need to use both PR's. My is meant for developers, so that we have all exactly the same versions and shrinkwrap is needed for travis. If I misunderstood shrinkwrap and when you run npm i shrinkwrap will be used and not package.json then my PR is not needed.

cc @bbondy

@diracdeltas
Copy link
Member

@NejcZdovc my understanding based on https://docs.npmjs.com/cli/shrinkwrap is that shrinkwrap is used for npm install.

@bsclifton
Copy link
Member

Closing in favor of #7795

@bsclifton
Copy link
Member

@diracdeltas that is correct- npm install will respect shrinkwrap 😄

@luixxiul
Copy link
Contributor

luixxiul commented Jun 9, 2017

On the latest npm, the file package-lock.json is produced after npm i. Should we add the file to .gitignore?

@NejcZdovc
Copy link
Contributor Author

We can for now, but as soon travis supports npm 5 we will use it for caching

@bbondy bbondy reopened this Jun 23, 2017
@bbondy
Copy link
Member

bbondy commented Jun 23, 2017

Re-opening for npm5

bbondy added a commit that referenced this issue Jun 23, 2017
@bbondy bbondy mentioned this issue Jun 23, 2017
8 tasks
@luixxiul luixxiul added needs-info Another team member needs information from the PR/issue opener. and removed QA/no-qa-needed release-notes/include labels Jul 31, 2017
evq referenced this issue Jul 31, 2017
Auditors: @evq

Test Plan:
1. Open about:preferences#plugins
2. Make sure `Adobe` and `wiki` appears as anchor links
3. Open about:preferences#payments
4. Disable payments
5. Make sure `View the FAQ` appears as an anchor link
@luixxiul luixxiul removed the needs-info Another team member needs information from the PR/issue opener. label Aug 30, 2017
@bsclifton bsclifton added this to the Triage Backlog milestone Nov 27, 2017
@NejcZdovc NejcZdovc removed their assignment Jan 12, 2018
@bsclifton
Copy link
Member

We've been using the lock files and these have worked out great 😄 👍

@bsclifton bsclifton removed this from the Triage Backlog milestone Aug 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants