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

[DevOverlay] Align old and new overlay #74935

Conversation

devjiwonchoi
Copy link
Member

@devjiwonchoi devjiwonchoi commented Jan 15, 2025

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:

Closes NDX-674
Closes NDX-687

Copy link
Member Author

devjiwonchoi commented Jan 15, 2025

@ijjk
Copy link
Member

ijjk commented Jan 15, 2025

Failing test suites

Commit: 2334de2

pnpm test-dev-turbo test/e2e/app-dir/app-compilation/index.test.ts (turbopack)

  • app dir > HMR > should not cause error when removing loading.js
Expand output

● app dir › HMR › should not cause error when removing loading.js

expect(received).toInclude(expected)

Expected string to include:
  "✓ Compiled"
Received:
  " ○ Compiling /page-with-loading ...
"

  38 |
  39 |         await retry(async () => {
> 40 |           expect(next.cliOutput.slice(cliOutputLength)).toInclude('✓ Compiled')
     |                                                         ^
  41 |         })
  42 |
  43 |         // It should not have an error

  at toInclude (e2e/app-dir/app-compilation/index.test.ts:40:57)
  at fn (lib/next-test-utils.ts:810:20)
  at Object.<anonymous> (e2e/app-dir/app-compilation/index.test.ts:39:9)

Read more about building and testing Next.js in contributing.md.

@ijjk
Copy link
Member

ijjk commented Jan 15, 2025

Stats from current PR

Default Build
General 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 ⚠️ +8.8 kB
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
Commit: 2334de2

@ijjk ijjk added the tests label Jan 16, 2025
@devjiwonchoi devjiwonchoi force-pushed the 01-15-_devoverlay_gather_feedback_per_error branch 3 times, most recently from 279cbd2 to 003a61d Compare January 16, 2025 16:43
@devjiwonchoi devjiwonchoi changed the base branch from 01-15-_devoverlay_gather_feedback_per_error to graphite-base/74935 January 16, 2025 17:00
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from 7d9c834 to f477735 Compare January 16, 2025 17:13
@devjiwonchoi devjiwonchoi changed the base branch from graphite-base/74935 to canary January 16, 2025 17:13
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from f477735 to ee2754d Compare January 16, 2025 17:18
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from ee2754d to 7c8e142 Compare January 16, 2025 19:32
@devjiwonchoi devjiwonchoi changed the base branch from canary to 01-17-_devoverlay_decouple_error_overlay_with_devtools_indicator January 16, 2025 19:32
@devjiwonchoi devjiwonchoi force-pushed the 01-17-_devoverlay_decouple_error_overlay_with_devtools_indicator branch from c671e8a to ddf157e Compare January 16, 2025 21:14
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from 7c8e142 to 7b6ba6e Compare January 16, 2025 21:14
@devjiwonchoi devjiwonchoi force-pushed the 01-17-_devoverlay_decouple_error_overlay_with_devtools_indicator branch 2 times, most recently from 970daea to 8bea48c Compare January 16, 2025 21:40
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from 7b6ba6e to e6d41c4 Compare January 16, 2025 21:40
@devjiwonchoi devjiwonchoi force-pushed the 01-17-_devoverlay_decouple_error_overlay_with_devtools_indicator branch from 8bea48c to ac0f76f Compare January 17, 2025 00:38
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch 2 times, most recently from 6649e86 to f2f1850 Compare January 17, 2025 00:52
@devjiwonchoi devjiwonchoi force-pushed the 01-17-_devoverlay_decouple_error_overlay_with_devtools_indicator branch from 0d43d19 to 05b4b00 Compare January 17, 2025 08:53
@devjiwonchoi devjiwonchoi changed the base branch from 01-17-_devoverlay_open_error_overlay_when_devtools_indicator_clicked to graphite-base/74935 January 21, 2025 05:02
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from b435757 to e3528dc Compare January 21, 2025 05:02
@devjiwonchoi devjiwonchoi changed the base branch from graphite-base/74935 to canary January 21, 2025 05:02
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch 2 times, most recently from b638ddd to c420ed7 Compare January 22, 2025 17:20
line
.substring(a + 1)
.replace(`^\\ {${miniLeadingSpacesLength}}`, '')
line.substring(a).replace(`^\\ {${miniLeadingSpacesLength}}`, '')
Copy link
Member Author

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.substring(a).replace(`^\\ {${miniLeadingSpacesLength}}`, '')

{getFrameSource(stackFrame)} @{' '}
<HotlinkedText text={stackFrame.methodName} />
{getFrameSource(stackFrame)}
{/* TODO: Unlike the CodeFrame component, the `methodName` is unavailable. */}
Copy link
Member Author

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.

{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)
Copy link
Member Author

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.

@@ -33,7 +33,7 @@ export function RuntimeError({ error }: RuntimeErrorProps) {
? []
: filteredFrames.slice(0, firstFirstPartyFrameIndex),
trailingCallStackFrames: filteredFrames.slice(
firstFirstPartyFrameIndex + 1
firstFirstPartyFrameIndex < 0 ? 0 : firstFirstPartyFrameIndex
Copy link
Member Author

@devjiwonchoi devjiwonchoi Jan 22, 2025

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.

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}
Copy link
Member Author

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.

@devjiwonchoi devjiwonchoi marked this pull request as ready for review January 22, 2025 17:54
>
{children}
</DevToolsErrorBoundary>
const devTools = (
Copy link
Member

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.

@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch 2 times, most recently from a46dd39 to 581c376 Compare January 23, 2025 06:36
@eps1lon eps1lon changed the title [DevOverlay] Enable new UI when PPR testing is enabled [DevOverlay] Align old and new overlay Jan 23, 2025
Copy link
Member

@eps1lon eps1lon left a 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

@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from 3c09148 to e12030b Compare January 23, 2025 14:39
@devjiwonchoi devjiwonchoi changed the base branch from canary to 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled January 23, 2025 14:39
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch 2 times, most recently from 288c3d0 to eff3089 Compare January 23, 2025 15:27
p.shadowRoot.querySelector(
// TODO(jiwon): data-nextjs-toast may not be an error indicator in new UI
process.env.__NEXT_EXPERIMENTAL_PPR
? '[data-issues]'
Copy link
Member

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?

Copy link
Member Author

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.

test/lib/next-test-utils.ts Outdated Show resolved Hide resolved
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' ||
Copy link
Member

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

Copy link
Member Author

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.

Base automatically changed from 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled to canary January 23, 2025 21:07
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from 655b867 to 2334de2 Compare January 23, 2025 21:17
@devjiwonchoi devjiwonchoi merged commit baa4f78 into canary Jan 23, 2025
129 of 131 checks passed
@devjiwonchoi devjiwonchoi deleted the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch January 23, 2025 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants