-
Notifications
You must be signed in to change notification settings - Fork 27.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
[DevOverlay] Align old and new overlay #74935
[DevOverlay] Align old and new overlay #74935
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Failing test suitesCommit: 2334de2
Expand output● app dir › HMR › should not cause error when removing loading.js
Read more about building and testing Next.js in contributing.md. |
Stats from current PRDefault BuildGeneral Overall increase
|
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
buildDuration | 19.2s | 17.6s | N/A |
buildDurationCached | 16.8s | 14.1s | N/A |
nodeModulesSize | 419 MB | 419 MB | |
nextStartRea..uration (ms) | 447ms | 446ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
5306-HASH.js gzip | 54.1 kB | 54.1 kB | N/A |
8276.HASH.js gzip | 169 B | 168 B | N/A |
8377-HASH.js gzip | 5.46 kB | 5.46 kB | N/A |
bccd1874-HASH.js gzip | 52.9 kB | 52.9 kB | N/A |
framework-HASH.js gzip | 57.5 kB | 57.5 kB | N/A |
main-app-HASH.js gzip | 241 B | 242 B | N/A |
main-HASH.js gzip | 34.6 kB | 34.6 kB | N/A |
webpack-HASH.js gzip | 1.71 kB | 1.71 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 39.4 kB | 39.4 kB | ✓ |
Overall change | 39.4 kB | 39.4 kB | ✓ |
Client Pages
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
_app-HASH.js gzip | 193 B | 193 B | ✓ |
_error-HASH.js gzip | 193 B | 193 B | ✓ |
amp-HASH.js gzip | 512 B | 510 B | N/A |
css-HASH.js gzip | 343 B | 342 B | N/A |
dynamic-HASH.js gzip | 1.84 kB | 1.84 kB | ✓ |
edge-ssr-HASH.js gzip | 265 B | 265 B | ✓ |
head-HASH.js gzip | 363 B | 362 B | N/A |
hooks-HASH.js gzip | 393 B | 392 B | N/A |
image-HASH.js gzip | 4.59 kB | 4.58 kB | N/A |
index-HASH.js gzip | 268 B | 268 B | ✓ |
link-HASH.js gzip | 2.35 kB | 2.35 kB | N/A |
routerDirect..HASH.js gzip | 328 B | 328 B | ✓ |
script-HASH.js gzip | 397 B | 397 B | ✓ |
withRouter-HASH.js gzip | 323 B | 326 B | N/A |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 3.59 kB | 3.59 kB | ✓ |
Client Build Manifests
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
_buildManifest.js gzip | 748 B | 747 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
index.html gzip | 523 B | 524 B | N/A |
link.html gzip | 539 B | 537 B | N/A |
withRouter.html gzip | 520 B | 520 B | ✓ |
Overall change | 520 B | 520 B | ✓ |
Edge SSR bundle Size
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
edge-ssr.js gzip | 129 kB | 129 kB | N/A |
page.js gzip | 210 kB | 210 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Middleware size
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 670 B | 666 B | N/A |
middleware-r..fest.js gzip | 155 B | 156 B | N/A |
middleware.js gzip | 31.3 kB | 31.3 kB | N/A |
edge-runtime..pack.js gzip | 844 B | 844 B | ✓ |
Overall change | 844 B | 844 B | ✓ |
Next Runtimes
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
274-experime...dev.js gzip | 322 B | 322 B | ✓ |
274.runtime.dev.js gzip | 314 B | 314 B | ✓ |
app-page-exp...dev.js gzip | 378 kB | 378 kB | N/A |
app-page-exp..prod.js gzip | 132 kB | 132 kB | ✓ |
app-page-tur..prod.js gzip | 145 kB | 145 kB | ✓ |
app-page-tur..prod.js gzip | 141 kB | 141 kB | ✓ |
app-page.run...dev.js gzip | 365 kB | 365 kB | N/A |
app-page.run..prod.js gzip | 128 kB | 128 kB | ✓ |
app-route-ex...dev.js gzip | 37.6 kB | 37.6 kB | ✓ |
app-route-ex..prod.js gzip | 25.6 kB | 25.6 kB | ✓ |
app-route-tu..prod.js gzip | 25.6 kB | 25.6 kB | ✓ |
app-route-tu..prod.js gzip | 25.4 kB | 25.4 kB | ✓ |
app-route.ru...dev.js gzip | 39.2 kB | 39.2 kB | ✓ |
app-route.ru..prod.js gzip | 25.4 kB | 25.4 kB | ✓ |
pages-api-tu..prod.js gzip | 9.69 kB | 9.69 kB | ✓ |
pages-api.ru...dev.js gzip | 11.6 kB | 11.6 kB | ✓ |
pages-api.ru..prod.js gzip | 9.68 kB | 9.68 kB | ✓ |
pages-turbo...prod.js gzip | 21.9 kB | 21.9 kB | ✓ |
pages.runtim...dev.js gzip | 27.7 kB | 27.7 kB | ✓ |
pages.runtim..prod.js gzip | 21.9 kB | 21.9 kB | ✓ |
server.runti..prod.js gzip | 916 kB | 916 kB | ✓ |
Overall change | 1.74 MB | 1.74 MB | ✓ |
build cache
vercel/next.js canary | vercel/next.js 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled | Change | |
---|---|---|---|
0.pack gzip | 2.1 MB | 2.1 MB | N/A |
index.pack gzip | 74.9 kB | 74.9 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Diff details
Diff for main-HASH.js
Diff too large to display
Diff for app-page-exp..ntime.dev.js
failed to diff
Diff for app-page.runtime.dev.js
failed to diff
279cbd2
to
003a61d
Compare
7d9c834
to
f477735
Compare
003a61d
to
4a0c516
Compare
f477735
to
ee2754d
Compare
ee2754d
to
7c8e142
Compare
c671e8a
to
ddf157e
Compare
7c8e142
to
7b6ba6e
Compare
970daea
to
8bea48c
Compare
7b6ba6e
to
e6d41c4
Compare
8bea48c
to
ac0f76f
Compare
6649e86
to
f2f1850
Compare
0d43d19
to
05b4b00
Compare
e207dfa
to
dbd5e1b
Compare
b435757
to
e3528dc
Compare
b638ddd
to
c420ed7
Compare
line | ||
.substring(a + 1) | ||
.replace(`^\\ {${miniLeadingSpacesLength}}`, '') | ||
line.substring(a).replace(`^\\ {${miniLeadingSpacesLength}}`, '') |
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.
Don't remove |
from the code frame after the line number, as it increases too many conditional snapshot testings. This is the original behavior from the old UI.
Line 37 in ab87453
line.substring(a).replace(`^\\ {${miniLeadingSpacesLength}}`, '') |
{getFrameSource(stackFrame)} @{' '} | ||
<HotlinkedText text={stackFrame.methodName} /> | ||
{getFrameSource(stackFrame)} | ||
{/* TODO: Unlike the CodeFrame component, the `methodName` is unavailable. */} |
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.
Remove unnecessary @
from the Terminal component, as we won't know the method name from the build error. This is the behavior from the old UI.
Lines 75 to 82 in ab87453
{file && ( | |
<EditorLink | |
isSourceFile | |
key={file.fileName} | |
file={file.fileName} | |
location={file.location} | |
/> | |
)} |
|
||
const { readyErrors } = useErrorHook({ | ||
errors: state.errors, | ||
isAppDir: false, | ||
}) | ||
|
||
const [isErrorOverlayOpen, setIsErrorOverlayOpen] = useState(true) |
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.
Set default value of isErrorOverlayOpen
to true. This is the behavior from the old UI.
next.js/packages/next/src/client/components/react-dev-overlay/pages/old-react-dev-overlay.tsx
Line 48 in ab87453
initialDisplayState={'fullscreen'} |
@@ -33,7 +33,7 @@ export function RuntimeError({ error }: RuntimeErrorProps) { | |||
? [] | |||
: filteredFrames.slice(0, firstFirstPartyFrameIndex), | |||
trailingCallStackFrames: filteredFrames.slice( | |||
firstFirstPartyFrameIndex + 1 | |||
firstFirstPartyFrameIndex < 0 ? 0 : firstFirstPartyFrameIndex |
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 is the old UI, backporting the behavior to include the first first-party call stack frame to be included in the CallStack component. Previously, it was only displayed at the CodeFrame.
Lines 12 to 23 in ab87453
const { firstFrame } = useMemo(() => { | |
const firstFirstPartyFrameIndex = error.frames.findIndex( | |
(entry) => | |
!entry.ignored && | |
Boolean(entry.originalCodeFrame) && | |
Boolean(entry.originalStackFrame) | |
) | |
return { | |
firstFrame: error.frames[firstFirstPartyFrameIndex] ?? null, | |
} | |
}, [error.frames]) |
</> | ||
return ( | ||
<DevToolsErrorBoundary | ||
devTools={devTools} |
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.
Should pass the devTools
to the error boundary when global error.
> | ||
{children} | ||
</DevToolsErrorBoundary> | ||
const devTools = ( |
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 should call it error overlay as it's only containing overlay related thing.
a46dd39
to
581c376
Compare
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.
Adjusted PR title. Previous one made it sound like it's testing only
3c09148
to
e12030b
Compare
288c3d0
to
eff3089
Compare
test/lib/next-test-utils.ts
Outdated
p.shadowRoot.querySelector( | ||
// TODO(jiwon): data-nextjs-toast may not be an error indicator in new UI | ||
process.env.__NEXT_EXPERIMENTAL_PPR | ||
? '[data-issues]' |
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.
why this is different on PPR? can we just align the data attributes?
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.
On old, on click toast opened the overlay, now need to click the correct element that has "open issue" action, since there's "dismiss" button included. __NEXT_EXPERIMENTAL_PPR
is a mistake.
config.experimental.newDevOverlay ?? false, | ||
// When `__NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY` is set on CI, | ||
// we need to pass it here so it can be enabled. | ||
process.env.__NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY === 'true' || |
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.
why we need another flag __NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY
set in CI, shouldn't it just be __NEXT_EXPERIMENTAL_PPR === true
? the variable on nextjs side is all set by define plugin
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.
x-ref: #74935 (comment)
__NEXT_EXPERIMENTAL_PPR
is feasible as it's only for testing, but I think explicit __NEXT_EXPERIMENTAL_NEW_DEV_OVERLAY
would be better to isolate the condition in case needed.
655b867
to
2334de2
Compare
Enable the new UI for the CI testings of existing redbox tests.
There are several changes made to let the test pass, including backporting changes to the old UI or removing one from the new, and are as follows:
|
after line number in code frame (link)@
in Terminal component (link)true
in Pages Router (link)Closes NDX-674
Closes NDX-687