-
Notifications
You must be signed in to change notification settings - Fork 64
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
Handle chunkLoadError #771
Conversation
✔️ Deploy Preview for storeui ready! 🔨 Explore the source changes: 81c08f8 🔍 Inspect the deploy log: https://app.netlify.com/sites/storeui/deploys/60c8ff27dd5722000871574d 😎 Browse the preview: https://deploy-preview-771--storeui.netlify.app |
de4d43c
to
5d3866c
Compare
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 81c08f8:
|
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.
it would be nice if we had a flag that checks process.env.NODE_ENV to disable this when in development
because this is only useful in production.
Like that:
// ...
useEffect(() => {
if (process.env.NODE_ENV === "production") handleError({ error, errorId })
}, [error, errorId])
// ...
} else if (is404) { | ||
window.location.href = `/404?from=${from}` | ||
} else { | ||
window.location.href = `/500?from=${from}&errorId=${errorId ?? uuidv4()}` |
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 need to add sentry here, in an agnostic way to the store can configure a own sentry
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.
Marin does this, but we still don't do in all of our stores. https://github.com/vtex-sites/marinbrasil.store/blob/master/src/%40vtex/gatsby-theme-store/components/Error/ErrorHandler.tsx
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.
yes, but we only generate a random id with uuid when the store does not shadowing this file.
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.
Yes. AFAIK sentry is open source right?
This was like this before, but I removed this
|
I tested the preview and it works as expected in my case. Previously it was easy to reproduce. Can't reproduce it anymore in this PR. |
What's the purpose of this pull request?
Fix being redirected to 500 page on a ChunkLoadError.
How it works?
Today, whenever an error happens, we redirect the user to the 500 page. If the user refreshs way too fast or if the file is not available on the server anymore, webpack throws a
ChunkLoadError
and the user is redirected to the 500 page.This PR solves this problem by refreshing the page when a ChunkLoadError happens on the React tree.
How to test it?
Refresh like crazy the home page before and after this PR. You will notice that eventually, you will be redirected to the 500 page. After this PR this shouldn't happen
https://github.com/vtex-sites/marinbrasil.store/pull/524
https://github.com/vtex-sites/btglobal.store/pull/626