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

Speed up CI #1076

Merged
merged 9 commits into from
Oct 23, 2016
Merged

Speed up CI #1076

merged 9 commits into from
Oct 23, 2016

Conversation

novemberborn
Copy link
Member

Inspired by https://twitter.com/kentcdodds/status/786282951210262528 I looked into caching npm and node_modules, which led to some more improvements:

  • Stop testing Node.js v5
  • In Travis, only install latest npm if the Node.js version is older than v6 (though I suppose when v4 ships we may want to change that again, or actually force it to install v3)
  • Cache npm cache and node_modules in both Travis and Appveyor
  • Clean up Appveyor config a little

On Travis the first, non-cached run took 5m59s. The second run took 4m15s. I think it saves on nyc instrumentation too since that's cached in node_modules. On Appveyor the time went from 13m3s to 9m30s (note that Appveyor doesn't run builds in parallel for us).

@vadimdemedes
Copy link
Contributor

vadimdemedes commented Oct 14, 2016

sindresorhus
sindresorhus previously approved these changes Oct 14, 2016
@jamestalmage
Copy link
Contributor

I definitely wouldn't cache node_modules. If deps are removed from package.json, they won't be cleared. This introduces initial state to the build, meaning CI no longer verifies it will install from scratch.

I am not sure about $HOME/.npm. If caching it does mask any problems, it seems certain those problems will be with npm.

@jamestalmage
Copy link
Contributor

The speedup is drastic enough on Appveyor that it's probably worth it there. I'd avoid it on Travis though.

before_install:
- 'npm install -g npm@latest'
- 'node -e "process.exit(Number(process.version.match(/^v(\d+)/)[1])>=6?0:1)" || npm install -g npm@latest'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can simplify this:

Number(process.version.match(/^v(\d+)/)[1])

into:

Number(process.version.slice(1)[0])

Copy link
Member Author

Choose a reason for hiding this comment

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

That'll break in 2018 when Node.js v10 ships. I'd rather just do it properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.

@novemberborn
Copy link
Member Author

I definitely wouldn't cache node_modules. If deps are removed from package.json, they won't be cleared. This introduces initial state to the build, meaning CI no longer verifies it will install from scratch.

npm still verifies all declared dependencies are present though. We could end up deleting a dependency that we still use, yes, but we have a similar problem with deduped dependencies.

Perhaps we could run npm prune after installation?

vadimdemedes
vadimdemedes previously approved these changes Oct 15, 2016
@sindresorhus
Copy link
Member

We could end up deleting a dependency that we still use, yes, but we have a similar problem with deduped dependencies.

The latest XO prevents this through the import/no-unresolved rule.

@novemberborn
Copy link
Member Author

The latest XO prevents this through the import/no-unresolved rule.

That should help. Worth pruning then? Are you OK with caching in Travis given #1080?

@sindresorhus
Copy link
Member

I'm still sceptical to caching the node_modules folder, but I'm open to trying it out and see how it works out. As long as we use np when publishing, we're certain we'll at least never publish a broken version. And yes, let's do pruning.

Ensure top-level dependencies are indeed no longer available when
removed from the package.json.
Soon npm@latest will be npm@4. Let's stick to v3 for now.
@novemberborn novemberborn dismissed stale reviews from sindresorhus and vadimdemedes October 23, 2016 15:56

Now pinning npm version, is that OK?

@sindresorhus sindresorhus merged commit e0a170b into master Oct 23, 2016
@sindresorhus sindresorhus deleted the improve-travis-caching branch October 23, 2016 16:47
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

Successfully merging this pull request may close these issues.

4 participants