Skip to content
This repository has been archived by the owner on Oct 14, 2020. It is now read-only.

Speed up ESLint + Prettier #175

Open
pdehaan opened this issue May 9, 2018 · 6 comments
Open

Speed up ESLint + Prettier #175

pdehaan opened this issue May 9, 2018 · 6 comments
Labels
improve-engineering

Comments

@pdehaan
Copy link
Collaborator

pdehaan commented May 9, 2018 •
edited
Loading

The majestic Mr @gregglind was commenting that the prettier and eslint --fix were a bit slow and was wondering if we could somehow optimize...

Filing this issue as a conversation starter and dumping ground for research findings and possible solutions.
@pdehaan
Copy link
Collaborator Author

pdehaan commented May 9, 2018

Here's some initial perf numbers and debug info from $ npm run eslint:

$ time DEBUG=eslint:cli-engine npm run eslint

> shield-studies-addon-utils@5.0.0-alpha1 eslint /Users/pdehaan/dev/github/mozilla/shield-studies-addon-utils
> eslint . --ext js --ext json

  eslint:cli-engine Linting ./.eslintrc.js +3ms
  eslint:cli-engine Linting ./bin/.eslintrc.js +0ms
  eslint:cli-engine Linting ./bin/copyStudyUtils.js +0ms
  eslint:cli-engine Linting ./bin/documentSchema.js +1ms
  eslint:cli-engine Linting ./bin/schemaToInterface.js +1ms
  eslint:cli-engine Linting ./bin/verifyWeeSchema.js +0ms
  eslint:cli-engine Linting ./bin/wee-schema-schema.json +5ms
  eslint:cli-engine Linting ./examples/small-study/src/.eslintrc.js +2ms
  eslint:cli-engine Linting ./examples/small-study/src/background.js +8ms
  eslint:cli-engine Linting ./examples/small-study/src/manifest.json +10ms
  eslint:cli-engine Linting ./examples/small-study/src/studySetup.js +3ms
  eslint:cli-engine Linting ./examples/small-study/web-ext-config.js +1ms
  eslint:cli-engine Linting ./test-addon/src/.eslintrc.js +5ms
  eslint:cli-engine Linting ./test-addon/src/background.js +3ms
  eslint:cli-engine Linting ./test-addon/src/extension-page-for-tests/page.js +1ms
  eslint:cli-engine Linting ./test-addon/src/manifest.json +1ms
  eslint:cli-engine Linting ./test-addon/src/studySetup.js +0ms
  eslint:cli-engine Linting ./test-addon/web-ext-config.js +0ms
  eslint:cli-engine Linting ./test/functional/browser.study.api.js +1ms
  eslint:cli-engine Linting ./test/functional/test-addon.js +0ms
  eslint:cli-engine Linting ./test/functional/utils.js +0ms
  eslint:cli-engine Linting ./testUtils/.eslintrc.js +0ms
  eslint:cli-engine Linting ./testUtils/executeJs.js +1ms
  eslint:cli-engine Linting ./testUtils/nav.js +0ms
  eslint:cli-engine Linting ./testUtils/setupWebdriver.js +0ms
  eslint:cli-engine Linting ./testUtils/telemetry.js +0ms
  eslint:cli-engine Linting ./testUtils/ui.js +1ms
  eslint:cli-engine Linting ./testUtils/wip.js +1ms
  eslint:cli-engine Linting ./webExtensionApis/prefs/api.js +1ms
  eslint:cli-engine Linting ./webExtensionApis/prefs/schema.json +0ms
  eslint:cli-engine Linting ./webExtensionApis/study/fakeApi.js +1ms
  eslint:cli-engine Linting ./webExtensionApis/study/schema.json +1ms
  eslint:cli-engine Linting ./webExtensionApis/study/src/index.js +1ms
  eslint:cli-engine Linting ./webExtensionApis/study/src/jsonschema.js +1ms
  eslint:cli-engine Linting ./webExtensionApis/study/src/sampling.js +0ms
  eslint:cli-engine Linting ./webExtensionApis/study/src/studyUtils.js +1ms
  eslint:cli-engine Linting ./webExtensionApis/study/src/studyUtilsBootstrap.js +1ms
  eslint:cli-engine Linting ./webExtensionApis/study/src/telemetry.js +1ms
  eslint:cli-engine Linting ./webExtensionApis/study/webpack.config.js +0ms
  eslint:cli-engine Linting complete in: 1547ms +6ms

...

DEBUG=eslint:cli-engine npm run eslint  2.58s user 0.42s system 79% cpu 3.757 total

$ git rev-parse --short HEAD # 9208966 ("develop" branch)

So it looks like simply running $ npm run eslint takes somewhere between 1.547 and 3.757 seconds. Nothing immediately stands out as being hugely offensive (performance wise). Most files are linted in 1-3ms, with a few outliers at 5ms, 8ms, and 10ms; which all seem reasonable.

Not sure if there is anything else we may want to add to the .eslintignore file:

$ cat .eslintignore

# do not lint/format generated artifacts
dist/
webExtensionApis/study/api.js
package-lock.json
# do not lint/format bundled util libraries
examples/test-addon/src/privileged/prefs/
examples/test-addon/src/privileged/study/
# makes sure that eslintrc.js gets linted/formatted
!.eslintrc.js
# don't lint/format code the directory where get-firefox stores the Firefox binaries
/firefox
# don't lint/format package.json since npm install formats it differently by default
package.json
# don't lint node_modules
node_modules

# re #169, skip the misc dir
misc/

@gregglind
Copy link
Contributor

gregglind commented May 9, 2018 via email

@pdehaan
Copy link
Collaborator Author

pdehaan commented May 10, 2018

Running $ npm run format, takes around 6-7 seconds.

✗ time npm run format

> shield-studies-addon-utils@5.0.0-alpha1 format /Users/pdehaan/dev/github/mozilla/shield-studies-addon-utils
> prettier '**/*.{css,js,jsm,json,md}' --trailing-comma=all --ignore-path=.eslintignore --write

.eslintrc.js 68ms
bin/.eslintrc.js 4ms
bin/copyStudyUtils.js 30ms
bin/documentSchema.js 42ms
bin/schemaToInterface.js 22ms
bin/verifyWeeSchema.js 35ms
bin/wee-schema-schema.json 20ms
docs/api.md 201ms
docs/development-on-the-utils.md 14ms
docs/engineering.md 211ms
docs/survival.md 57ms
docs/tutorial.md 22ms
docs2.md 134ms
examples/small-study/README.md 13ms
examples/small-study/src/.eslintrc.js 4ms
examples/small-study/src/background.js 18ms
examples/small-study/src/manifest.json 13ms
examples/small-study/src/studySetup.js 24ms
examples/small-study/web-ext-config.js 5ms
README.md 65ms
test-addon/src/.eslintrc.js 3ms
test-addon/src/background.js 23ms
test-addon/src/extension-page-for-tests/page.js 3ms
test-addon/src/manifest.json 6ms
test-addon/src/studySetup.js 20ms
test-addon/web-ext-config.js 4ms
test/functional/browser.study.api.js 80ms
test/functional/test-addon.js 21ms
test/functional/utils.js 6ms
testUtils/.eslintrc.js 2ms
testUtils/executeJs.js 12ms
testUtils/nav.js 4ms
testUtils/setupWebdriver.js 19ms
testUtils/telemetry.js 18ms
testUtils/ui.js 36ms
testUtils/wip.js 10ms
webExtensionApis/prefs/api.js 4ms
webExtensionApis/prefs/schema.json 8ms
webExtensionApis/study/fakeApi.js 36ms
webExtensionApis/study/schema.json 44ms
webExtensionApis/study/src/index.js 60ms
webExtensionApis/study/src/jsonschema.js 11ms
webExtensionApis/study/src/sampling.js 17ms
webExtensionApis/study/src/studyUtils.js 74ms
webExtensionApis/study/src/studyUtilsBootstrap.js 25ms
webExtensionApis/study/src/telemetry.js 18ms
webExtensionApis/study/webpack.config.js 5ms

> shield-studies-addon-utils@5.0.0-alpha1 postformat /Users/pdehaan/dev/github/mozilla/shield-studies-addon-utils
> run-p lint:fixpack eslint-fix


> shield-studies-addon-utils@5.0.0-alpha1 eslint-fix /Users/pdehaan/dev/github/mozilla/shield-studies-addon-utils
> npm run eslint -- --fix


> shield-studies-addon-utils@5.0.0-alpha1 lint:fixpack /Users/pdehaan/dev/github/mozilla/shield-studies-addon-utils
> fixpack  # cleans up package.json

package.json already clean!

> shield-studies-addon-utils@5.0.0-alpha1 eslint /Users/pdehaan/dev/github/mozilla/shield-studies-addon-utils
> eslint . --ext js --ext json "--fix"

...

✖ 21 problems (0 errors, 21 warnings)

npm run format  6.54s user 1.00s system 102% cpu 7.334 total

It's a bit hard to tell, but if I re-run npm run format over and over, we seem to have a small handful of files that will get modified by prettier and then rewritten by eslint --fix:

  1. bin/schemaToInterface.js
  2. test/functional/browser.study.api.js
  3. test/functional/test-addon.js
  4. testUtils/executeJs.js
  5. testUtils/nav.js
  6. testUtils/setupWebdriver.js
  7. testUtils/telemetry.js
  8. testUtils/ui.js

I'd probably remove fixpack, but doubt that it'd speed it up considerably.

@pdehaan
Copy link
Collaborator Author

pdehaan commented May 10, 2018 •
edited
Loading

Another option we could look at is something like husky + lint-staged and only lint the modified files which would speed things up exponentially (and automatically format+lint files on precommit or prepush git hooks).

UPDATE: I'm starting to wonder how great an idea this is. 💡

We would only run npm run format on a precommit or prepush git hook, which would probably speed it up greatly. Similarly, using lint-staged we only run fixpack whenever the package.json file has changed, since otherwise it would be an expensive noop.

Another possible optimization would be replacing calls w/ the expensive npm run build with npm run fast-build, which does the same thing, but without the prebuild and postbuild lifecycle hooks:

    "prepare": "fixpack && npm run build",
    "presmall-study:run": "npm run build && npm run small-study:bundle-utils",
    "pretest": "npm run build && npm run test-addon:bundle-utils && npm run test-addon:build",

Where the prebuild and postbuild currently look like:

    "postbuild": "if [ -z ${SKIPLINT} ]; then npm run format; fi",
    "postformat": "run-p lint:fixpack eslint-fix",
    "prebuild": "if [ -z ${SKIPLINT} ]; then npm run lint; fi",

So, the current flow of running something like $ npm test would do something like:

  1. $ npm test:
    1. runs pretest script which calls npm run build.
      1. runs prebuild script which calls npm run lint (unless passed a SKIPLINT env var).
        1. npm run lint runs ESLint and fixpack.
      2. runs build script which does stuff.
      3. runs postbuild which calls npm run format (unless passed a SKIPLINT env var).
        1. npm run format calls prettier to reformat entire code base (*.css, *.js, *.jsm, *.json, and *.md files).
        2. runs postformat which runs fixpack (again) and ESLint (again) with --fix flag to reformat code base (again).
    2. runs test script which calls mocha and runs the actual tests.

@pdehaan
Copy link
Collaborator Author

pdehaan commented May 10, 2018

I ran npm run format (without any other npm lifecycle hooks), and it looks like the only diffs were stuff like the following:

--- a/testUtils/nav.js
+++ b/testUtils/nav.js
@@ -4,7 +4,7 @@ const firefox = require("selenium-webdriver/firefox");
 const Context = firefox.Context;

 module.exports.nav = {
-  gotoURL: async(driver, url) => {
+  gotoURL: async (driver, url) => {

@pdehaan
Copy link
Collaborator Author

pdehaan commented May 10, 2018

Another option is something like prettier-eslint, which:

Formats your JavaScript using prettier followed by eslint --fix

I haven't tried it before, so can't comment on efficiency or how the configs work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
improve-engineering
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gregglind @pdehaan @motin