-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
feat(react-dev-overlay): export getErrorByType and add preventDisplay prop #34237
feat(react-dev-overlay): export getErrorByType and add preventDisplay prop #34237
Conversation
This comment has been minimized.
This comment has been minimized.
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.
Instead of emitting a new event and hiding the current RuntimeError
component, would it make sense to allow passing in a new component as a prop which is used inside of the RuntimeError
component?
@ijjk I think there's better ways to do this. In Next Live the app is running in an iframe and then we actually are passing the error / stack frame up to the parent page and rendering it there, so passing in a Component wouldn't work for this. We also are keeping compilation errors displaying as they are currently in react-dev-overlay (for now, at least). We may want to in the future also pull those out. Re-reading react-dev-overlay, I think I can actually just export the Then the only thing we'd want is some sort of prop to disable rendering on runtime errors. Perhaps Alternatively I could try something a big hackier that doesn't even require a PR like listening on the bus and applying some CSS to hide it. Both feel a bit lacking. |
89df91e
to
ca302c7
Compare
This comment has been minimized.
This comment has been minimized.
@ijjk Updated this, now it's much simpler. Just exposes |
1f959fe
to
3e7799e
Compare
which errors are display (used by next live)
3e7799e
to
b755484
Compare
A bit confused, I'm working off
|
# Conflicts: # packages/next/compiled/@next/react-dev-overlay/client.js
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.
Looks good
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Stats from current PRDefault Build (Decrease detected ✓)General Overall increase
|
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
buildDuration | 16s | 15.8s | -195ms |
buildDurationCached | 6.3s | 6.1s | -169ms |
nodeModulesSize | 367 MB | 367 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 2.987 | 3 | |
/ avg req/sec | 837.07 | 833.43 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.364 | 1.38 | |
/error-in-render avg req/sec | 1832.64 | 1811.25 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42 kB | 42 kB | ✓ |
main-HASH.js gzip | 27.9 kB | 27.9 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71.6 kB | 71.6 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.36 kB | 1.36 kB | ✓ |
_error-HASH.js gzip | 194 B | 194 B | ✓ |
amp-HASH.js gzip | 312 B | 312 B | ✓ |
css-HASH.js gzip | 326 B | 326 B | ✓ |
dynamic-HASH.js gzip | 2.57 kB | 2.57 kB | ✓ |
head-HASH.js gzip | 350 B | 350 B | ✓ |
hooks-HASH.js gzip | 919 B | 919 B | ✓ |
image-HASH.js gzip | 5.05 kB | 5.05 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 2.26 kB | 2.26 kB | ✓ |
routerDirect..HASH.js gzip | 321 B | 321 B | ✓ |
script-HASH.js gzip | 383 B | 383 B | ✓ |
withRouter-HASH.js gzip | 318 B | 318 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 14.7 kB | 14.7 kB | ✓ |
Client Build Manifests
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
_buildManifest.js gzip | 460 B | 460 B | ✓ |
Overall change | 460 B | 460 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
index.html gzip | 532 B | 532 B | ✓ |
link.html gzip | 544 B | 544 B | ✓ |
withRouter.html gzip | 526 B | 526 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
Default Build with SWC (Increase detected ⚠️ )
General Overall increase ⚠️
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
buildDuration | 19.2s | 19.1s | -129ms |
buildDurationCached | 6.2s | 6.2s | |
nodeModulesSize | 367 MB | 367 MB |
Page Load Tests Overall increase ✓
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 2.99 | 2.952 | -0.04 |
/ avg req/sec | 836.01 | 846.94 | +10.93 |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.395 | 1.369 | -0.03 |
/error-in-render avg req/sec | 1791.64 | 1826.23 | +34.59 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.1 kB | 42.1 kB | ✓ |
main-HASH.js gzip | 27.9 kB | 27.9 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71.7 kB | 71.7 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
_error-HASH.js gzip | 180 B | 180 B | ✓ |
amp-HASH.js gzip | 305 B | 305 B | ✓ |
css-HASH.js gzip | 321 B | 321 B | ✓ |
dynamic-HASH.js gzip | 2.56 kB | 2.56 kB | ✓ |
head-HASH.js gzip | 342 B | 342 B | ✓ |
hooks-HASH.js gzip | 911 B | 911 B | ✓ |
image-HASH.js gzip | 5.08 kB | 5.08 kB | ✓ |
index-HASH.js gzip | 256 B | 256 B | ✓ |
link-HASH.js gzip | 2.28 kB | 2.28 kB | ✓ |
routerDirect..HASH.js gzip | 314 B | 314 B | ✓ |
script-HASH.js gzip | 375 B | 375 B | ✓ |
withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 14.7 kB | 14.7 kB | ✓ |
Client Build Manifests
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 459 B | ✓ |
Overall change | 459 B | 459 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | natew/next.js nate/react-dev-overlay-error-reporting | Change | |
---|---|---|---|
index.html gzip | 533 B | 533 B | ✓ |
link.html gzip | 547 B | 547 B | ✓ |
withRouter.html gzip | 527 B | 527 B | ✓ |
Overall change | 1.61 kB | 1.61 kB | ✓ |
Report full parsed runtime errors over error bus, accepts preventDisplay prop to avoid showing error messages inline, while still reporting errors over the bus.
Basically, we want to handle the parsed error/stack differently in Next Live, showing a modal that sits above the content and allows users to report the issue to us. We want to have that stack trace in the issue report, so I added a new event
unhandled-error-full
.The
preventDisplay
prop then just lets us output our own modal instead of showing the error inline, but still renders the<RuntimeErrors />
component so it can fetch the stack and report it over the bus.This isn't beautiful code per-se, but I think doing it really right would require a pretty intense re-structure of this module. I think ideally we'd have export a function to fetch of the stack that we can just expose separately - that fetch currently happens in a sub-sub-component (DevOverlay > Errors > RuntimeError). But that re-write is pretty high effort, would still require much of what we do here anyway, and would just to get a slightly less awkward API in a not very high-use area. So leaving it as-is for now, happy to revisit though if we want.
Feature
fixes #number
Fixes an issue with Next Live #290.
Documentation / Examples
yarn lint