-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Run npm update and npm install on CircleCI #1685
Conversation
The Maybe deleting the cached |
@@ -5,6 +5,8 @@ dependencies: | |||
pre: | |||
- sudo apt-get update; sudo apt-get -y install python-pip libglew-dev gdb | |||
- sudo pip install awscli | |||
override: | |||
- npm update && npm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are both update
and install
required? The docs say update "will also install missing packages".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. The one difference is that npm update
doesn't run any of the package.json
scripts (preinstall
, postinstall
, prepublish
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not using any of those scripts right now but disabling them feels like a big "gotcha" waiting to happen. Not sure how to weigh that cost against the increased build time.
I don't have a strong gut feeling on |
My gut feeling is that we should try the |
a87f208
to
73b15ff
Compare
On further thought, lets skip "job security" |
73b15ff
to
829d6e8
Compare
Ok. I think the latest iteration gets it right. We run 🚢 @jfirebaugh? |
👍 |
Merged in 9db3c0a |
Because CircleCI uses a cached
node_modules
folder and never runsnpm update
, it doesn't grab the latest versions of dependencies. This PR causes CircleCI to runnpm update
alongsidenpm install
on every build.Is this approach preferable to using shrinkwrap #1607?
fixes #1600
cc @jfirebaugh @tmcw @mourner