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

Add caching ability to the loader #93

Merged
merged 4 commits into from
Jul 28, 2016
Merged

Add caching ability to the loader #93

merged 4 commits into from
Jul 28, 2016

Conversation

genintho
Copy link
Contributor

@genintho genintho commented Jun 16, 2016

While webpack is great to pick up only incremental change, the compilation time at lunch can be very long. For our application, it takes minutes, which means it takes minutes before our tests can run, before we can deploy, before developer can start working when starting their day.

I did similar changes to the coffee-loader, or the Uglify plugin, saving huge chunk of time.

Cache the eslint result into a file.
The input is hash using MD5 to detect changes.

{
    "path.js": {
       "hash":"7c451761c8712467357cd5750e303d1f",
       "res": "what_ever"
}

This cut 30% of my webpack build time.

If this PR is accepted, I am happy to update the documentation and write a few tests.

Fixes #82

@genintho
Copy link
Contributor Author

Please ignore the CI status, I will fix this if the idea behind the PR is approved.

@MoOx
Copy link
Contributor

MoOx commented Jun 16, 2016

Maybe you can post some benchs (without cache, with cache)?
For the cache folder, it can be nice to simply use https://www.npmjs.com/package/find-cache-dir
If benchs prove to be useful, then I am ok with this feature.

@genintho
Copy link
Contributor Author

Eslint process 220 files
screen shot 2016-06-16 at 11 30 42 am

run 1: no cache
run 2: no cache
run 3: cache on -> compute it
run 4: cache on
run 5: cache on

The more file you process, the bigger the wins are.

@genintho
Copy link
Contributor Author

I updated the PR too

@MoOx
Copy link
Contributor

MoOx commented Jun 17, 2016

Looks good!
Can you add some doc in the README please?

@genintho
Copy link
Contributor Author

I will. It will be a little delayed, I am in vacation for now :)

@MoOx
Copy link
Contributor

MoOx commented Jun 23, 2016

@genintho np, enjoy ;)

@genintho
Copy link
Contributor Author

Documentation added.

@genintho genintho changed the title [Prototype] - Add caching ability to the loader Add caching ability to the loader Jul 16, 2016
try {
var cacheFileContent = fs.readFileSync(cachePath)
if (cacheFileContent) {
cache = JSON.parse(cacheFileContent)
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason you don't require() the file instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure. Is it better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Better I can't prove it, but definitely shorter :)

This should be enough:

try { 
  cache = require(thunk("data.json"))
}
catch (e) {
  cache = {}
}

@genintho
Copy link
Contributor Author

updated.

@MoOx
Copy link
Contributor

MoOx commented Jul 28, 2016

Thanks. Sorry for the delay. Will cut a release.

@MoOx MoOx merged commit d05c92c into webpack-contrib:master Jul 28, 2016
@genintho
Copy link
Contributor Author

Woot!

@genintho
Copy link
Contributor Author

genintho commented Aug 3, 2016

So one piece of feedback regarding the usage of find-cache-dir. The library is not really solid as the path of the cache directory it find depends on where you are when you run the webpack process. So doing things like node ~/oneDir/2ndDir/3rdDir/myconfig/build.js is not caching at the same location (and in fact can throw errors) than doing node myconfig/build.js .

@MoOx
Copy link
Contributor

MoOx commented Aug 3, 2016

@genintho maybe we can add a way to specify a folder? Default to find-cache-dir, and ability to specify one?

@gaearon
Copy link

gaearon commented Aug 4, 2016

I think it would make sense to use the same caching strategy as babel-loader uses since it’s been tested thoroughly.

@MoOx
Copy link
Contributor

MoOx commented Aug 4, 2016

Indeed! Would be nice to have this as a module ;)

@gaearon
Copy link

gaearon commented Feb 27, 2017

Unfortunately, this implementation misses some of the essential error handling that babel-loader provides. Would you be interested in fixing this? I posted a small analysis in facebook/create-react-app#1656 (comment) after this caused issues for Create React App users when we enabled it by default.

@MoOx
Copy link
Contributor

MoOx commented Feb 27, 2017

I personally don't have time for this but maybe someone else will. Feel free to open an issue as a reminder until someone handle it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants