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] Enable reactOwnerStack when newDevOverlay is enabled #75199

Conversation

devjiwonchoi
Copy link
Member

@devjiwonchoi devjiwonchoi commented Jan 22, 2025

Enable reactOwnerStack when newDevOverlay is enabled to have better call stack output in the new UI.

Closes NDX-668

Copy link
Member Author

devjiwonchoi commented Jan 22, 2025

@devjiwonchoi devjiwonchoi requested a review from eps1lon January 22, 2025 19:00
@devjiwonchoi devjiwonchoi marked this pull request as ready for review January 22, 2025 19:00
@devjiwonchoi devjiwonchoi removed the request for review from eps1lon January 22, 2025 19:00
@ijjk
Copy link
Member

ijjk commented Jan 22, 2025

Failing test suites

Commit: b66a0be

pnpm test test/integration/amp-export-validation/test/index.test.js

  • AMP Validation on Export > production mode > should have shown errors during build
Expand output

● AMP Validation on Export › production mode › should have shown errors during build

expect(received).toMatch(expected)

Expected pattern: /error.*The mandatory attribute 'height' is missing in tag 'amp-video'\./
Received string:  "   Loading config from /root/actions-runner/_work/next.js/next.js/test/integration/amp-export-validation/next.config.js
   Loading config from /root/actions-runner/_work/next.js/next.js/test/integration/amp-export-validation/next.config.js
   ▲ Next.js 15.2.0-canary.21·
   Checking validity of types ...
   Creating an optimized production build ...
   Loading config from /root/actions-runner/_work/next.js/next.js/test/integration/amp-export-validation/next.config.js
   Loading config from /root/actions-runner/_work/next.js/next.js/test/integration/amp-export-validation/next.config.js
   Loading config from /root/actions-runner/_work/next.js/next.js/test/integration/amp-export-validation/next.config.js
 ✓ Compiled successfully
   Collecting page data ...
   Generating static pages (0/8) ...
   Generating static pages (2/8)··
 ⚠ Linting is disabled.
Error occurred prerendering page \"/third\". Read more: https://nextjs.org/docs/messages/prerender-error
AssertionError: Assertion failed: WebAssembly is uninitialized

  at new module$contents$goog$asserts_AssertionError (../evalmachine.<anonymous>:106:1695)
  at module$contents$goog$asserts_doAssertFailure (../evalmachine.<anonymous>:107:354)
  at goog.asserts.assertExists (../evalmachine.<anonymous>:109:142)
  at Object.module$contents$amp$validator_validateString [as validateString] (../evalmachine.<anonymous>:2663:108)
  at Validator.validateString (../packages/next/dist/compiled/amphtml-validator/index.js:1:20650)
  at validateAmp (../packages/next/dist/export/routes/pages.js:87:34)
  at async exportPagesPage (../packages/next/dist/export/routes/pages.js:121:17)
  at async Span.traceAsyncFn (../packages/next/dist/trace/trace.js:156:20)
  at async exportPage (../packages/next/dist/export/worker.js:345:18)
  Export encountered an error on /third, exiting the build.
   ⨯ Static worker exited with code: 1 and signal: null
  "
  at Object.toMatch (integration/amp-export-validation/test/index.test.js:28:29)

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

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:806: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 22, 2025

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled Change
buildDuration 20.1s 17.1s N/A
buildDurationCached 16.4s 14s N/A
nodeModulesSize 419 MB 419 MB ⚠️ +3.24 kB
nextStartRea..uration (ms) 436ms 417ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_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-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_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-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_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-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_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-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled Change
index.html gzip 523 B 522 B N/A
link.html gzip 538 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-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled Change
edge-ssr.js gzip 129 kB 129 kB N/A
page.js gzip 209 kB 209 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled Change
middleware-b..fest.js gzip 670 B 667 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-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_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 376 kB 376 kB
app-page-exp..prod.js gzip 131 kB 131 kB
app-page-tur..prod.js gzip 144 kB 144 kB
app-page-tur..prod.js gzip 140 kB 140 kB
app-page.run...dev.js gzip 364 kB 364 kB
app-page.run..prod.js gzip 127 kB 127 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 2.48 MB 2.48 MB
build cache
vercel/next.js canary vercel/next.js 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled Change
0.pack gzip 2.11 MB 2.1 MB N/A
index.pack gzip 75.5 kB 75.3 kB N/A
Overall change 0 B 0 B
Diff details
Diff for main-HASH.js

Diff too large to display

Commit: b66a0be

@devjiwonchoi devjiwonchoi force-pushed the 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled branch 2 times, most recently from 0da8462 to 17a0ba9 Compare January 22, 2025 19:23
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from 57cb856 to a46dd39 Compare January 23, 2025 03:50
@devjiwonchoi devjiwonchoi force-pushed the 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled branch from 17a0ba9 to c875271 Compare January 23, 2025 06:05
@devjiwonchoi devjiwonchoi force-pushed the 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled branch from a46dd39 to 581c376 Compare January 23, 2025 06:36
@devjiwonchoi devjiwonchoi force-pushed the 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled branch 2 times, most recently from d39acec to 9a8f1cd Compare January 23, 2025 07:28
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.

Missing on the Turbopack side. It's also not the same thing. Instead, we need to make sure the default values for these flags change.

@devjiwonchoi devjiwonchoi force-pushed the 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled branch 2 times, most recently from fa73365 to 623325b Compare January 23, 2025 10:56
@devjiwonchoi devjiwonchoi force-pushed the 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled branch from 623325b to d66d83b Compare January 23, 2025 14:39
@devjiwonchoi devjiwonchoi changed the base branch from 01-16-_devoverlay_enable_new_ui_when_ppr_testing_is_enabled to canary January 23, 2025 14:39
@@ -1234,7 +1234,14 @@ export const defaultConfig: NextConfig = {
staticGenerationMinPagesPerWorker: 25,
dynamicIO: false,
inlineCss: false,
newDevOverlay: false,
// TODO: remove once we've made new UI default
get newDevOverlay() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default value, it should be false. turning on the flag should be done in load config.
The config will also be serialized in output manifest, I'd prefer we keep the default config flag and simple

devjiwonchoi and others added 2 commits January 24, 2025 00:22
Co-authored-by: Jiachi Liu <inbox@huozhi.im>
Co-authored-by: Jiachi Liu <inbox@huozhi.im>
Copy link

vercel bot commented Jan 23, 2025

Notifying the following users due to files changed in this PR based on this repo's notify modifiers:

@timneutkens, @ijjk, @shuding, @huozhi:

packages/next/src/server/config.ts

Copy link
Member

@huozhi huozhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel we should have a test for this but not super blocking

@devjiwonchoi devjiwonchoi merged commit 2f0c80f into canary Jan 23, 2025
130 of 132 checks passed
@devjiwonchoi devjiwonchoi deleted the 01-23-_devoverlay_enable_reactownerstack_when_newdevoverlay_is_enabled branch January 23, 2025 21:07
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.

4 participants