-
Notifications
You must be signed in to change notification settings - Fork 5k
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: Reload the app when there is a javascript error and a new version of the app #3715
Conversation
This pull request is being automatically deployed with Vercel (learn more). interface – ./🔍 Inspect: https://vercel.com/uniswap/interface/8DV2xDnPjgBgP4Y47KHQgTz4LRZc widgets – ./🔍 Inspect: https://vercel.com/uniswap/widgets/BLJEoMLnNjcrhUzN8G5dU5r2AKcU donate – ./🔍 Inspect: https://vercel.com/uniswap/donate/F6nyYCj56BvzFf9eE4NW3i7KA3Hg |
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 looks good! i know you said you were able to test this locally - how hard do you think it would be to add a unit test? or do you think it makes more sense to try to land tests once the fully featured service worker flow lands?
@NoahZinsmeister the local testing I did involved building and serving the binary and then using the devtools to force an update so it doesn't translate easily to unit tests. I'd prefer to wait until we decide what to do with the rest of the service worker and then I can implement unit tests for the whole thing. |
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.
The code looks great, can you just clean up the comments? Normally I wouldn't be this nitpicky but SW is a little esoteric, so I think it's important to document inline and link out to supporting docs (most of which you've already done 😄).
if (registration?.waiting) { | ||
await registration.unregister() | ||
|
||
// Makes Workbox call skipWaiting() |
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.
Can you link to documentation in this comment? This isn't a typed API, so there's no real inline documentation.
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 included a link to the workbox documentation that talks about skipWaiting and why its necessary. I think this should address this comment and the one below.
The initial implementation by @moodysalem was done in: #3435. The problem there was if there was no update from the app it would get stuck refreshing infinitely.
I copied that PR but added a check to ensure that there is actually a new version being returned from the update and only in that case will the app update. I also had to add a few more lines that appear to be required to make the refresh happen properly which I copied from: facebook/create-react-app#5316 (comment)