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] Fix Style Regression #74768

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

devjiwonchoi
Copy link
Member

@devjiwonchoi devjiwonchoi commented Jan 10, 2025

Why?

There were regressions in both the floating-header story and the dialog border.

Before

CleanShot 2025-01-11 at 05 51 51

After

CleanShot 2025-01-11 at 05 51 44

Floating Header Story

This was invisible due to translateY.

CleanShot 2025-01-11 at 05 49 59

Closes NDX-654

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Jan 10, 2025
Copy link
Member Author

devjiwonchoi commented Jan 10, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ijjk
Copy link
Member

ijjk commented Jan 10, 2025

Failing test suites

Commit: 00ccbb6

pnpm test-dev-turbo test/e2e/app-dir/non-root-project-monorepo/non-root-project-monorepo.test.ts (turbopack)

  • non-root-project-monorepo > import.meta.url > should work during RSC
  • non-root-project-monorepo > import.meta.url > should work during SSR
  • non-root-project-monorepo > import.meta.url > should work on client-side
  • non-root-project-monorepo > monorepo-package > should work during RSC
  • non-root-project-monorepo > monorepo-package > should work during SSR
  • non-root-project-monorepo > monorepo-package > should work on client-side
  • non-root-project-monorepo > source-maps > should work on RSC
  • non-root-project-monorepo > source-maps > should work on SSR
  • non-root-project-monorepo > source-maps > should work on client-side
Expand output

● non-root-project-monorepo › monorepo-package › should work during RSC

Failed to start server after 10000ms, waiting for this log pattern: / ✓ Ready in /

  370 |     return setTimeout(() => {
  371 |       reject(
> 372 |         new Error(
      |         ^
  373 |           `Failed to start server after ${ms}ms, waiting for this log pattern: ${this.serverReadyPattern}`
  374 |         )
  375 |       )

  at Timeout._onTimeout (lib/next-modes/base.ts:372:9)

● non-root-project-monorepo › monorepo-package › should work during SSR

Failed to start server after 10000ms, waiting for this log pattern: / ✓ Ready in /

  370 |     return setTimeout(() => {
  371 |       reject(
> 372 |         new Error(
      |         ^
  373 |           `Failed to start server after ${ms}ms, waiting for this log pattern: ${this.serverReadyPattern}`
  374 |         )
  375 |       )

  at Timeout._onTimeout (lib/next-modes/base.ts:372:9)

● non-root-project-monorepo › monorepo-package › should work on client-side

Failed to start server after 10000ms, waiting for this log pattern: / ✓ Ready in /

  370 |     return setTimeout(() => {
  371 |       reject(
> 372 |         new Error(
      |         ^
  373 |           `Failed to start server after ${ms}ms, waiting for this log pattern: ${this.serverReadyPattern}`
  374 |         )
  375 |       )

  at Timeout._onTimeout (lib/next-modes/base.ts:372:9)

● non-root-project-monorepo › import.meta.url › should work during RSC

Failed to start server after 10000ms, waiting for this log pattern: / ✓ Ready in /

  370 |     return setTimeout(() => {
  371 |       reject(
> 372 |         new Error(
      |         ^
  373 |           `Failed to start server after ${ms}ms, waiting for this log pattern: ${this.serverReadyPattern}`
  374 |         )
  375 |       )

  at Timeout._onTimeout (lib/next-modes/base.ts:372:9)

● non-root-project-monorepo › import.meta.url › should work during SSR

Failed to start server after 10000ms, waiting for this log pattern: / ✓ Ready in /

  370 |     return setTimeout(() => {
  371 |       reject(
> 372 |         new Error(
      |         ^
  373 |           `Failed to start server after ${ms}ms, waiting for this log pattern: ${this.serverReadyPattern}`
  374 |         )
  375 |       )

  at Timeout._onTimeout (lib/next-modes/base.ts:372:9)

● non-root-project-monorepo › import.meta.url › should work on client-side

Failed to start server after 10000ms, waiting for this log pattern: / ✓ Ready in /

  370 |     return setTimeout(() => {
  371 |       reject(
> 372 |         new Error(
      |         ^
  373 |           `Failed to start server after ${ms}ms, waiting for this log pattern: ${this.serverReadyPattern}`
  374 |         )
  375 |       )

  at Timeout._onTimeout (lib/next-modes/base.ts:372:9)

● non-root-project-monorepo › source-maps › should work on RSC

Failed to start server after 10000ms, waiting for this log pattern: / ✓ Ready in /

  370 |     return setTimeout(() => {
  371 |       reject(
> 372 |         new Error(
      |         ^
  373 |           `Failed to start server after ${ms}ms, waiting for this log pattern: ${this.serverReadyPattern}`
  374 |         )
  375 |       )

  at Timeout._onTimeout (lib/next-modes/base.ts:372:9)

● non-root-project-monorepo › source-maps › should work on SSR

Failed to start server after 10000ms, waiting for this log pattern: / ✓ Ready in /

  370 |     return setTimeout(() => {
  371 |       reject(
> 372 |         new Error(
      |         ^
  373 |           `Failed to start server after ${ms}ms, waiting for this log pattern: ${this.serverReadyPattern}`
  374 |         )
  375 |       )

  at Timeout._onTimeout (lib/next-modes/base.ts:372:9)

● non-root-project-monorepo › source-maps › should work on client-side

Failed to start server after 10000ms, waiting for this log pattern: / ✓ Ready in /

  370 |     return setTimeout(() => {
  371 |       reject(
> 372 |         new Error(
      |         ^
  373 |           `Failed to start server after ${ms}ms, waiting for this log pattern: ${this.serverReadyPattern}`
  374 |         )
  375 |       )

  at Timeout._onTimeout (lib/next-modes/base.ts:372:9)

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

pnpm test-dev test/e2e/app-dir/actions/app-action.test.ts

  • app-dir action handling > should forward action request to a worker that contains the action handler (node)
  • app-dir action handling > should forward action request to a worker that contains the action handler (edge)
  • app-dir action handling > should not error when a forwarded action triggers a redirect (node)
  • app-dir action handling > fetch actions > should handle redirects to routes that provide an invalid RSC response
Expand output

● app-dir action handling › should forward action request to a worker that contains the action handler (node)

page.waitForSelector: Timeout 60000ms exceeded.
Call log:
  - waiting for locator('#other-page')

  423 |     return this.chain(() => {
  424 |       return page
> 425 |         .waitForSelector(selector, { timeout, state: 'attached' })
      |          ^
  426 |         .then(async (el) => {
  427 |           // it seems selenium waits longer and tests rely on this behavior
  428 |           // so we wait for the load event fire before returning

  at waitForSelector (lib/browsers/playwright.ts:425:10)
  at e2e/app-dir/actions/app-action.test.ts:862:7
  at Proxy.chain (lib/browsers/base.ts:17:23)
  at Proxy.chain (lib/browsers/playwright.ts:423:17)
  at waitForElementByCss (e2e/app-dir/actions/app-action.test.ts:865:10)

● app-dir action handling › should forward action request to a worker that contains the action handler (edge)

page.waitForSelector: Timeout 60000ms exceeded.
Call log:
  - waiting for locator('#other-page')

  423 |     return this.chain(() => {
  424 |       return page
> 425 |         .waitForSelector(selector, { timeout, state: 'attached' })
      |          ^
  426 |         .then(async (el) => {
  427 |           // it seems selenium waits longer and tests rely on this behavior
  428 |           // so we wait for the load event fire before returning

  at waitForSelector (lib/browsers/playwright.ts:425:10)
  at e2e/app-dir/actions/app-action.test.ts:862:7
  at Proxy.chain (lib/browsers/base.ts:17:23)
  at Proxy.chain (lib/browsers/playwright.ts:423:17)
  at waitForElementByCss (e2e/app-dir/actions/app-action.test.ts:865:10)

● app-dir action handling › should not error when a forwarded action triggers a redirect (node)

page.waitForSelector: Timeout 60000ms exceeded.
Call log:
  - waiting for locator('#other-page')

  423 |     return this.chain(() => {
  424 |       return page
> 425 |         .waitForSelector(selector, { timeout, state: 'attached' })
      |          ^
  426 |         .then(async (el) => {
  427 |           // it seems selenium waits longer and tests rely on this behavior
  428 |           // so we wait for the load event fire before returning

  at waitForSelector (lib/browsers/playwright.ts:425:10)
  at e2e/app-dir/actions/app-action.test.ts:905:7
  at Proxy.chain (lib/browsers/base.ts:17:23)
  at Proxy.chain (lib/browsers/playwright.ts:423:17)
  at waitForElementByCss (e2e/app-dir/actions/app-action.test.ts:908:10)

● app-dir action handling › fetch actions › should handle redirects to routes that provide an invalid RSC response

expect(received).toContain(expected) // indexOf

Expected substring: "Hello from a pages route"
Received string:    "0.8561258110392751
Client
Server
Client and Server
0
+1+1 (Slow)-1*2
redirect to a pages route
submit
test"

  1269 |
  1270 |       await retry(async () => {
> 1271 |         expect(await browser.elementByCss('body').text()).toContain(
       |                                                           ^
  1272 |           'Hello from a pages route'
  1273 |         )
  1274 |         expect(await browser.url()).toBe(`${next.url}/pages-dir`)

  at toContain (e2e/app-dir/actions/app-action.test.ts:1271:59)
  at retry (lib/next-test-utils.ts:806:14)
  at Object.<anonymous> (e2e/app-dir/actions/app-action.test.ts:1270:7)

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

@devjiwonchoi devjiwonchoi marked this pull request as ready for review January 10, 2025 20:55
@devjiwonchoi devjiwonchoi requested review from ijjk and gaojude January 10, 2025 20:55
@devjiwonchoi devjiwonchoi changed the base branch from 01-11-_devoverlay_improve_storybook_structure to graphite-base/74768 January 11, 2025 13:26
@devjiwonchoi devjiwonchoi force-pushed the 01-11-_devoverlay_fix_style_regression branch from 7842432 to db9aee5 Compare January 11, 2025 13:27
@devjiwonchoi devjiwonchoi changed the base branch from graphite-base/74768 to canary January 11, 2025 13:27
@devjiwonchoi devjiwonchoi force-pushed the 01-11-_devoverlay_fix_style_regression branch from db9aee5 to 00ccbb6 Compare January 11, 2025 13:28
@ijjk
Copy link
Member

ijjk commented Jan 11, 2025

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js 01-11-_devoverlay_fix_style_regression Change
buildDuration 22.3s 19.2s N/A
buildDurationCached 18.3s 15.4s N/A
nodeModulesSize 417 MB 417 MB ⚠️ +10.9 kB
nextStartRea..uration (ms) 517ms 511ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js 01-11-_devoverlay_fix_style_regression Change
5306-HASH.js gzip 53.3 kB 53.3 kB N/A
8276.HASH.js gzip 169 B 168 B N/A
8377-HASH.js gzip 5.44 kB 5.44 kB N/A
bccd1874-HASH.js gzip 53 kB 53 kB
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.2 kB 34.2 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB N/A
Overall change 53 kB 53 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js 01-11-_devoverlay_fix_style_regression 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-11-_devoverlay_fix_style_regression 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.57 kB 4.57 kB N/A
index-HASH.js gzip 268 B 268 B
link-HASH.js gzip 2.35 kB 2.34 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-11-_devoverlay_fix_style_regression Change
_buildManifest.js gzip 749 B 747 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js 01-11-_devoverlay_fix_style_regression Change
index.html gzip 523 B 524 B N/A
link.html gzip 537 B 538 B N/A
withRouter.html gzip 518 B 520 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js 01-11-_devoverlay_fix_style_regression Change
edge-ssr.js gzip 129 kB 129 kB N/A
page.js gzip 207 kB 207 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js 01-11-_devoverlay_fix_style_regression Change
middleware-b..fest.js gzip 670 B 668 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31.2 kB 31.2 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-11-_devoverlay_fix_style_regression 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 369 kB 369 kB N/A
app-page-exp..prod.js gzip 130 kB 130 kB
app-page-tur..prod.js gzip 142 kB 142 kB
app-page-tur..prod.js gzip 138 kB 138 kB
app-page.run...dev.js gzip 357 kB 357 kB N/A
app-page.run..prod.js gzip 126 kB 126 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.7 kB 21.7 kB
pages.runtim...dev.js gzip 27.5 kB 27.5 kB
pages.runtim..prod.js gzip 21.7 kB 21.7 kB
server.runti..prod.js gzip 916 kB 916 kB
Overall change 1.73 MB 1.73 MB
build cache
vercel/next.js canary vercel/next.js 01-11-_devoverlay_fix_style_regression Change
0.pack gzip 2.09 MB 2.09 MB N/A
index.pack gzip 75.4 kB 74.3 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: 00ccbb6

@devjiwonchoi devjiwonchoi requested review from huozhi and removed request for ijjk January 12, 2025 09:15
@devjiwonchoi devjiwonchoi merged commit 9fd30a5 into canary Jan 13, 2025
127 of 130 checks passed
@devjiwonchoi devjiwonchoi deleted the 01-11-_devoverlay_fix_style_regression branch January 13, 2025 00:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants