Create-react-app: Consider using eslint-loader `cache` option

Created on 24 Sep 2016  路  22Comments  路  Source: facebook/create-react-app

@kentcdodds mentioned he鈥檚 not using eslint-loader because of perf issues.
Nobody reported them to us, so naturally we don鈥檛 know about them.

What kind of perf issues exist with eslint-loader?
Can you tell me how to reproduce them?

Does enabling that cache option (new in 1.5.0) help, and should we do it?

bug underlying tools

Most helpful comment

It's fixed on 1.0.1. whew! broke 2 project release because I wanted to contribute to eslint caching... T_T

All 22 comments

cc @andreypopp who might have opinions

I don't use CRA with large codebases yet so I don't have perf issues with eslint-loader.

I assume that ESLint's cache option only helps initial linting because otherwise eslint-loader is marked as cacheable and so it lints only changed modules (when in watch mode).

Here's an example. I have experienced this myself on bigger projects. This is why I generally keep eslint out of my webpack stuff and have it as a separate script that I run as a pre-push hook.

@kentcdodds So the issue is in production builds, or initial npm start? This shouldn鈥檛 affect rebuild times in development as far as I understand.

cc @ericclemmons

@gaearon Unfortunately I can't provide the copy/paste metrics (they were lost in chat long ago, but I can give you near exact info)

  • Our typical app start time was ~18 seconds.
  • Re-builds (HMR) took ~3s, then ~2s after that. (No idea why sub-sequent re-builds get _faster_, but they stabilize at the 3rd or 4th re-build).
  • I went through every optional plugin (including my own, so totes unbiased) and did a webpack build 5x, and averaged the results.
  • No other plugin had a meaningful drop, except for eslint-loader.
  • (Note, we weren't aware of the cache option at the time, so this is _without it_)
  • When I removed eslint-loader as a preLoader:

    • Our cold npm start took ~12s (down from ~18s). This is because babel-loader is the 2nd most intensive process in the app.

    • Our warm npm start takes ~8s (just tested it repeatedly), now that most of what babel-loader does it cached.

  • Re-compiles (HMR) on the client take ~0.8-1.2s. They hovered around ~1.5-2.4s previously.

As a result, we did the following:

  • Remove eslint-loader from the compilation process. Caching may help, but personally we found that _stylistic errors shouldn't break the build_ while _syntax errors should_, but those are quite visible in the IDE anyway.
  • Lint as the first pass in CI, where _any_ output (errors/warnings alike) break the build.

Also, I originally implemented DLLPlugin to resolve speed issues. (You may remember me mentioning it in another CRA thread) Unfortunately, it simply required too much orchestration (especially as the app & deps grew) for the speed gains.

Knowing what I know now, for speeding up builds it would be:

  1. Let webpack handle _building_, and let validation happen elsewhere. (Webpack is optimstic and faster this way).
  2. Minimize babel usage. Meaning, if you're running Chrome/Node v6, _you can do an entire build without Babel, until you need JSX support_. (Of course there are exceptions, but the complete compile down to ES5 is unnecessary IMO for RAD)
  3. Reluctantly add DLLPlugin, as this is the closest you'll get to having your app be entirely just ./src.

I hope this helps! We _may_ try this again with cache: true in eslint-loader, but I just don't think it's worth it for me.

Released in 0.9.1/2.

Reopening since we reverted this in 0.9.3.
Waiting for https://github.com/MoOx/eslint-loader/pull/159 to get in.

Anybody wants to help me to write the tests and do code review? Currently it doesn't pass any of the tests T_T. I've published a fork here https://www.npmjs.com/package/eslint-loader-fs-cache if anybody wants to play with it and compare with the older implementation.

Can you post a more detailed note of what needs to be done? So that a person who wants to help knows where to start, what you tried, and what the desired outcome is. https://github.com/MoOx/eslint-loader/pull/159 is probably the best place to do this.

Can we punt this to 0.10 @gaearon? I don't want to risk additional breakages if we forget an edge case (even though using babel's module is a safe bet, but I don't want to rely on it without support from babel too [as in babel switches to the new module]). We might explore an entire webpack caching plugin in 0.10 which will nullify the need for this anyway.

Sure.

The latest release of eslint-loader is broken because its new dependency (https://github.com/viankakrisna/loader-fs-cache) uses mkdirp but doesn't declare it as a dependency: https://github.com/viankakrisna/loader-fs-cache/pull/2

It's fixed on 1.0.1. whew! broke 2 project release because I wanted to contribute to eslint caching... T_T

It happens to the best of us, @viankakrisna. 馃槃

@Timer shall we turn it on again in master? 馃槃

I learn so much in these mishaps though. It's both stressful and rewarding working on open source... :)

Also, i'm curious about the whole

We might explore an entire webpack caching plugin in 0.10 which will nullify the need for this anyway.

Is it dll? Or any other one?

We were looking at https://github.com/jsdf/cached-loader. As for re-enabling this, I am hesitant because I'm about to go on vacation and I don't want to release a build right before I leave, haha.

Woow, that's cool! Yeah, have a nice vacation! I agree if cached-loader can do it, it's better to have it rather than make caching scattered across loader.

Let's reevaluate later.

I tried putting this behind cache loader and it didn't give a speed improvement; we'll need to try the built in cache option now if it's stable. No harm in flipping the flag back on.

Going to close as stale.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

stopachka picture stopachka  路  3Comments

barcher picture barcher  路  3Comments

xgqfrms-GitHub picture xgqfrms-GitHub  路  3Comments

wereHamster picture wereHamster  路  3Comments

alleroux picture alleroux  路  3Comments