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

(Not Urgent) Improve Performance of CI #26

Open
ScottRudiger opened this issue Apr 28, 2019 · 1 comment
Open

(Not Urgent) Improve Performance of CI #26

ScottRudiger opened this issue Apr 28, 2019 · 1 comment

Comments

@ScottRudiger
Copy link
Contributor

ScottRudiger commented Apr 28, 2019

This is not urgent but something to keep in mind when there's time. Feel free to skip this for now.

There's a few ways we could improve performance for running builds in Travis CI.

  1. Currently, npm's prepublish script is being used to build before publishing. However, this causes CI to build dist/ twice for each node version: once on npm install and again on npm test.

Screen Shot 2019-04-27 at 3 00 06 PM
Screen Shot 2019-04-27 at 3 00 38 PM
(Note that tsc runs twice.)

This is because of the behavior of prepublish. It triggers whenever npm install is run within the project and on npm publish.

  • If we change it to prepublishOnly, then CI will only build once (before running tests) and the behavior of building before npm publish will be unchanged. (Note: the maintainer who publishes will need at least [email protected] for prepublishOnly to work.)

You can see the behavior by checking out this commit:

git fetch https://github.com/ScottRudiger/taxjar-node.git 02d0051c00d7323cd81907e33ab05f3eef539e4b:ci/improve-perf
git checkout ci/improve-perf
rm -r node_modules
npm install # no longer builds 'dist/'
npm test # still builds 'dist/'
npm publish --dry-run # still builds 'dist/'
  • Or, we could go even further and make it so CI still performs tests on dist/ but for a slightly better DX make it so dist/ is never built locally for common development tasks such as testing.

Here's a proof of concept. Mocha uses ts-node/register to compile for tests without building dist/ locally. But Travis CI sets the env var CI=true so the script

"test": "if [ $CI ]; then tsc && mocha; else mocha -r ts-node/register; fi"

should cause Travis to still build and test against dist/. (If we go ahead with this change, we can make this cleaner by instead defining that behavior in .travis.yml.)

Check it out locally with:

git checkout master
git branch -D ci/improve-perf
git fetch https://github.com/ScottRudiger/taxjar-node.git 91903275241833b887b0b0e06704b734dc4ed0c8:ci/improve-perf
git checkout ci/improve-perf
rm -r dist
npm install # no longer builds 'dist/'
npm test # no longer builds 'dist/'
CI=true npm test # builds 'dist/'
npm publish --dry-run # still builds 'dist/'
  1. Skip non-LTS versions of Node.

Instead of 4 through 10 we could skip the non-LTS Node versions for testing, except the latest. I think it’d be 6, 8, 10, 11 and maybe 12 since it was released a few days ago. Of course, we could keep 4 even though it’s out of LTS. The main idea is to skip 5, 7, and 9 to speed up the build.

  1. Also, look into having Travis cache node_modules or use npm cit on versions where it’s available.

  2. Other than that, I thought it might be useful to run eslint in the latest Node version and exit early if there are any linting errors. That should speed up CI significantly in those cases.

@ScottRudiger
Copy link
Contributor Author

Another benefit of running mocha with ts-node/register is that we can write tests using modern JavaScript features available after Node 4.

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

No branches or pull requests

1 participant