Skip to content
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

Merged
merged 3 commits into from
Apr 13, 2022

Conversation

leggechr
Copy link
Contributor

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)

@vercel
Copy link

vercel bot commented Apr 12, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

interface – ./

🔍 Inspect: https://vercel.com/uniswap/interface/8DV2xDnPjgBgP4Y47KHQgTz4LRZc
✅ Preview: https://interface-git-sw-error-reload-uniswap.vercel.app

widgets – ./

🔍 Inspect: https://vercel.com/uniswap/widgets/BLJEoMLnNjcrhUzN8G5dU5r2AKcU
✅ Preview: https://widgets-git-sw-error-reload-uniswap.vercel.app

donate – ./

🔍 Inspect: https://vercel.com/uniswap/donate/F6nyYCj56BvzFf9eE4NW3i7KA3Hg
✅ Preview: https://donate-git-sw-error-reload-uniswap.vercel.app

@leggechr leggechr changed the title Reload the app when there is a javascript error and a new version of the app Fix: Reload the app when there is a javascript error and a new version of the app Apr 12, 2022
@leggechr leggechr changed the title Fix: Reload the app when there is a javascript error and a new version of the app fix: Reload the app when there is a javascript error and a new version of the app Apr 12, 2022
Copy link
Contributor

@NoahZinsmeister NoahZinsmeister left a 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?

@leggechr
Copy link
Contributor Author

@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.

Copy link
Contributor

@zzmp zzmp left a 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 😄).

src/components/ErrorBoundary/index.tsx Outdated Show resolved Hide resolved
src/components/ErrorBoundary/index.tsx Show resolved Hide resolved
src/components/ErrorBoundary/index.tsx Outdated Show resolved Hide resolved
src/components/ErrorBoundary/index.tsx Outdated Show resolved Hide resolved
if (registration?.waiting) {
await registration.unregister()

// Makes Workbox call skipWaiting()
Copy link
Contributor

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.

Copy link
Contributor Author

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.

src/components/ErrorBoundary/index.tsx Outdated Show resolved Hide resolved
@leggechr leggechr merged commit cbe421e into main Apr 13, 2022
@leggechr leggechr deleted the sw-error-reload branch April 13, 2022 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants