Fix: File events with persistent cache #127
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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? |
ahdinosaur
added a commit
to ahdinosaur/node-browserify
that referenced
this pull request
Mar 20, 2017
|
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, |
goto-bus-stop
added a commit
to choojs/bankai
that referenced
this pull request
Feb 14, 2018
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.
goto-bus-stop
added a commit
to choojs/bankai
that referenced
this pull request
Feb 14, 2018
* 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sadly I introduced a bug when adding the persistent cache to module-deps in v4.1.0. 😥
The persistent cache doesn't send a
fileevent 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.sourceis given on L#350 and L#375 but sent to the callback in the thepersistentCacheFallbackon 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
fileevents 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