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

Use npm shrinkwrap and travis caching #7795

Merged
merged 2 commits into from
Apr 18, 2017
Merged

Use npm shrinkwrap and travis caching #7795

merged 2 commits into from
Apr 18, 2017

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Mar 20, 2017

fix #7820

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Test Plan:

  1. automated tests should not fail with a lint error

@bsclifton
Copy link
Member

bsclifton commented Mar 21, 2017

Docs for reference (I've never used shrinkwrap- need to look more into it) - https://docs.npmjs.com/cli/shrinkwrap

This would make the updating processing interesting. I'm also curious if it'll work OK across platforms (Windows, Linux, macOS)

@cezaraugusto
Copy link
Contributor

fyi I opened a PR to update standard@9 that could help as well: #7817 so repo/travis can share the same version

@diracdeltas diracdeltas changed the title Use npm shrinkwrap Use npm shrinkwrap and travis caching Mar 21, 2017
@cezaraugusto
Copy link
Contributor

sorry just saw Yan comment on ref PR. I'm curious why Travis isn't matching repo's deps version.

@bsclifton bsclifton mentioned this pull request Apr 13, 2017
@bsclifton
Copy link
Member

bsclifton commented Apr 18, 2017

@diracdeltas would you be able to run npm shrinkwrap --dev again against current master? (and commit/squash)

I think this PR solves the problem in a much more standard way than #8283. Sorry I had been hesitant to accept it- this would be a great one 😄

@bsclifton bsclifton requested a review from NejcZdovc April 18, 2017 06:05
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

Left a comment in #8283 (comment). I think that we need this PR for travis caching, so that we can speed up our test's.

@diracdeltas
Copy link
Member Author

updated shrinkwrap

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

++ 😄

@bsclifton bsclifton added this to the 0.15.1 milestone Apr 18, 2017
@bsclifton bsclifton merged commit e02260c into master Apr 18, 2017
@bsclifton bsclifton deleted the feature/shrinkwrap branch April 18, 2017 23:58
@NejcZdovc NejcZdovc mentioned this pull request Apr 19, 2017
4 tasks
@@ -22,6 +22,7 @@ Brave welcomes contributions via [pull requests](https://github.com/brave/browse
Fix #206
````

* If you update the npm dependencies, run `npm shrinkwrap --dev` before committing changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: is it ok to run that everytime when I commit something to keep the dependencies up to date?

Copy link
Member

Choose a reason for hiding this comment

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

@luixxiul we'll want to pick and choose the times we update dependencies. Unless there is a good reason (need new version to get a feature, we're too out of date, etc) , we should stick with the version there. This is important to stabilize the codebase.

Any updates we do to libraries we use may require a small regression test to ensure the upgrade didn't break any functionality or introduce issues

Copy link
Contributor

Choose a reason for hiding this comment

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

@bsclifton thanks!

bsclifton referenced this pull request Apr 21, 2017
With the fix to a non-existent self.state.properties

Fixes #8423
@luixxiul luixxiul removed this from the 0.15.300 milestone May 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

re-enable npm travis caching
5 participants