Port babel loader fs cache as the default caching engine #159
Conversation
viankakrisna
changed the title
|
Needs rebasing? |
viankakrisna
force-pushed
the
port_babel_loader_fs_cache
branch
2 times, most recently
from
3695876
to
fdbca0e
Compare
|
@gaearon done! |
|
Can't we try to use a common module instead of copy/pasting/adapting? |
|
It would be nice but as a follow up work IMO :-) We're hoping to fix this asap, and extracting a module would require coordination between both packages and somebody willing to do the work. |
|
🤔 |
|
ah, should i coordinate first? @gaearon |
|
If you already published it, I guess it's fine! Can you send a PR to |
|
@gaearon gladly! yes, essentially i just wrap the babel implementation with a function that takes the name for findCacheDir https://github.com/viankakrisna/loader-fs-cache/blob/master/index.js#L179 |
|
This is reported to work well. |
viankakrisna
force-pushed
the
port_babel_loader_fs_cache
branch
from
9dcef3c
to
847f5e0
Compare
viankakrisna
force-pushed
the
port_babel_loader_fs_cache
branch
from
847f5e0
to
e33ea6b
Compare
|
Unfortunately I don't know anything about the test setup in this repo, @MoOx can you help? |
|
@viankakrisna if you're having trouble with the tests hanging it's because you have a bunch of |
|
@wrakky ah, haven't pushed the updated codes! 😢 I'm trying to figure out why all the tests would fail, even when the cache option is turned off 😭 |
index.js
Outdated
| } | ||
| printLinterOutput(lint(engine, input, resourcePath), config, this) | ||
| this.callback(input, map) |
There was a problem hiding this comment.
@viankakrisna i think this is your problem - first argument to this.callback is an error so should be set to null like the other callback a couple of line above it.
|
@wrakky thank you! That works! I wonder why it's changed like that though 😅 |
|
@MoOx I think it should be good now 👍 |
viankakrisna
changed the title
|
It seems to be good for me, just one last question: I think by reading the tests that you are covering by tests things that should be taken care by the module that handle the cache (loader-fs-cache). |
|
@MoOx yeah, that's what I'm thinking too, do you have any particular line of the test that should be moved to loader-fs-cache? And how can we test the cache feature without leaking the implementation detail? |
| }) | ||
| }) | ||
|
|
||
| test.cb.serial("should output json.gz files to standard cache dir by default", |
There was a problem hiding this comment.
this is the kind of test that should live in the loader cache module.
|
Sorry my time is limited atm. Let's ship this as it is. |
|
Published as 1.7.0 |
|
Hi, this latest PR looks like it broke dependencies on |
This pr implements the tried and tested fs-cache from babel-loader. It still need to test some case though. The current implementation of caching is quite buggy based on the issue on facebook/create-react-app#1656 .
Todos:
loader-fs-cacheinstead of data.json and other use cases..Create benchmark and see the speed we gain by turning on cacheWill do it another timeFor people who want to test this pr I've published an npm package in https://www.npmjs.com/package/eslint-loader-fs-cache