-
-
Notifications
You must be signed in to change notification settings - Fork 121
Port babel loader fs cache as the default caching engine #159
Port babel loader fs cache as the default caching engine #159
Conversation
Needs rebasing? |
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. |
9dcef3c
to
847f5e0
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 👍 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-cache
instead 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