-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: File events with persistent cache #127
Fix: File events with persistent cache #127
Conversation
…n the persistent cache.
The addition of emitting the I imagine these changes would have be a major version bump, to ensure we don't break existing tools relying on the original behaviour? |
thanks for the detailed explanation! the error changes make sense to me. it seems like this works so i think we should just merge it, bump major, |
Cache transform results in `~/.cache/bankai` using [browserify-persist-fs](https://github.com/martinheidegger/browserify-persist-fs). This should not be merged until browserify/module-deps#127 is, since currently this does not work great with watchify (if I understand the linked PR correctly). Anyway, on the `./example` folder in this repo, results: ```bash $ time npm run build # cold run npm run build 13.79s user 0.35s system 159% cpu 8.843 total $ time npm run build # warm run npm run build 9.64s user 0.30s system 170% cpu 5.818 total ``` This is already a pretty big win, but on a larger app it should be much much more noticeable still.
* Persistent cache Cache transform results in `~/.cache/bankai` using [browserify-persist-fs](https://github.com/martinheidegger/browserify-persist-fs). This should not be merged until browserify/module-deps#127 is, since currently this does not work great with watchify (if I understand the linked PR correctly). Anyway, on the `./example` folder in this repo, results: ```bash $ time npm run build # cold run npm run build 13.79s user 0.35s system 159% cpu 8.843 total $ time npm run build # warm run npm run build 9.64s user 0.30s system 170% cpu 5.818 total ``` This is already a pretty big win, but on a larger app it should be much much more noticeable still. * Add babelify and preset to persistent cache key * Move persistent cache creation to separate file so we can run gc on build
* Persistent cache Cache transform results in `~/.cache/bankai` using [browserify-persist-fs](https://github.com/martinheidegger/browserify-persist-fs). This should not be merged until browserify/module-deps#127 is, since currently this does not work great with watchify (if I understand the linked PR correctly). Anyway, on the `./example` folder in this repo, results: ```bash $ time npm run build # cold run npm run build 13.79s user 0.35s system 159% cpu 8.843 total $ time npm run build # warm run npm run build 9.64s user 0.30s system 170% cpu 5.818 total ``` This is already a pretty big win, but on a larger app it should be much much more noticeable still. * Add babelify and preset to persistent cache key * Move persistent cache creation to separate file so we can run gc on build
Sadly I introduced a bug when adding the persistent cache to module-deps in v4.1.0. 😥
The persistent cache doesn't send a
file
event since that event would be triggered only in case the fallback was published.This PR fixes the event behaviour by sending the file and error event after checking the file in the cache. 💡
To maintain the order of the events, the errors now bubble through the
dup
(L#237, L#250) rather thanself
. This way the errors can be passed to the callback and thus is guaranteed to run after the file event L#388Since the processing of the error is now different for the two process routes: the errors have to be bubbled on when
rec.source
is given on L#350 and L#375 but sent to the callback in the thepersistentCacheFallback
on L#397 and L#403 instead of in readFile L#209After all that, the order should stay consistent but there are not enough tests for the correct order to feel 100% confident. (and I am not sure how to test all cases properly)
However... I didn't find a good way to preserve the current event timing. The
file
events will now bubble later than they did before, after the processing of the file. In many cases they will even be executed in the same tick because the file emit is in the same scope as the error emit L#388😅
I thought about moving them at least 1 tick apart with process.nextTick but I wondered if that is just a bit too cautious. 🤔
cc. @jesstelford