-
Notifications
You must be signed in to change notification settings - Fork 4.7k
chore: Prefetch consolidated api using service worker #33306
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
Merged
Merged
Changes from 5 commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
e0cacf1
WIP
c748784
WIP 2
ae3afec
WIP 3
38eba74
Add view url
74964a8
Move service worker to dev config
f985f17
Merge remote-tracking branch 'origin' into chore/prefetch-consolidate…
2cfc7e7
Add url origin to the prefetching logic
0ab583f
Revert changes in craco common
dd9f8e6
Revert a change in serviceWorker
5d27434
Make changes to invalidate current bundle
90580be
Interim change
411c77a
Reset the cache on first fetch
c32a137
Merge remote-tracking branch 'origin' into chore/prefetch-consolidate…
4d9c7b2
Refactor code to handle git applications also
3bf22d7
Move methods to a util fn
094de9d
Add test cases
ff376d6
Revert unnecessary changes
b9f1ab5
PR comments
02967d5
Refactor and update test cases
cf64171
Remove caching of files indev mode of service worker
2e6f9b5
Add version number to cache name
7284f81
Implement skip cache
9300be7
Final checks and test cases
72e079f
Fix a typo
feae9fa
Add comments
7c6ab9d
Add cache expiry
ae9aa5b
PR comments
cd75730
PR comments
7cb6ea6
Add comments and change variable name
a32c11f
Implement the logic with mutex
ceaf254
refactor
016c983
Update test cases
4024a6f
Remove logs and add comments
d06f4d9
Rename describe block
17b6d45
PR comments
30e2db9
Add catch block to main request handler
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
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.
Revert this. This change is required only for testing
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.
Does this change need to be reverted ?
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.
We can leave it as is and enable it for dev mode.
Just FYI there are some warnings that popup in the terminal as result of enabling WorkboxPlugin in dev mode. This is a known issue for webpack watch mode (InjectManifest has been called multiple times, perhaps due to running webpack in --watch mode.)
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.
@dvj1988 Does it affect the build time somehow? Did you check it?
We have some issues with building time, if it is not necessary, then I would prefer not to add anything here.
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.
I have not seen any issues with the dev server build time or hot reloads. Preferably we want to keep this so that we have the same behaviour of the service worker in the dev and prod modes.
Earlier we were using service worker to cache the static assets but this PR is prefetching and caching one of the apis. If there are any main bundle changes that would affect the behaviour of the service worker or vice versa we want to start catching them in the dev mode itself.
After the merge I will keep a track of whether this is affecting any devs local setup and can take a measure accordingly.
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.
@KelvinOm Do you have any specific scenario in mind to test?
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.
@dvj1988 I am most interested in the start time (with and without cache) and the recompilation time. We also have problems OOM issues, this should not make the situation worse.
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.
@KelvinOm By cache do you mean lint cache?
How do you propose I measure these?
Uh oh!
There was an error while loading. Please reload this page.
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.
By cache I mean
babel-loadercache it stored atapp/client/node_modules/.cache/babel-loader. It is created when the webpack is running.I do not know a good way to do this. I'm measuring the time in terminal. We can use
timeliketime yarn start. I couldn't run the webpack speed measure plugin because it doesn't work withcraco. So we have to measure it manually.Uh oh!
There was an error while loading. Please reload this page.
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.
@KelvinOm I used the package called
gnomon. And the time taken looks the same.Start time without cache was replicated by following the steps
yarn start | gnomon --type=elapsed-totalTests done in: Apple M1 Pro, RAM 32 GB
Start time without cache - release




Start time with cache - release
Start time without cache - chore/prefetch-consolidated-api
Start time with cache - chore/prefetch-consolidated-api