Include param vars in nearest scope bindings #29536

padmaia · 2021-09-30T22:03:08Z

This fixes an issue where some dynamic elements were not getting styles. See updated test for details.

kdy1

I found some small issues.

packages/next/build/swc/src/styled_jsx/mod.rs

ijjk · 2021-10-02T15:53:20Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
buildDuration	16s	16s	-45ms
buildDurationCached	3.9s	3.9s	⚠️ +59ms
nodeModulesSize	183 MB	183 MB	⚠️ +91.4 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.744	3.616	-0.13
/ avg req/sec	667.65	691.35	+23.7
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.065	2.108	⚠️ +0.04
/error-in-render avg req/sec	1210.91	1186.16	⚠️ -24.75

Client Bundles (main, webpack, commons)

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
779.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	26.9 kB	26.9 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	70.8 kB	70.8 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
_app-HASH.js gzip	977 B	977 B	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	311 B	311 B	✓
css-HASH.js gzip	328 B	328 B	✓
dynamic-HASH.js gzip	2.67 kB	2.67 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	918 B	918 B	✓
image-HASH.js gzip	4.15 kB	4.15 kB	✓
index-HASH.js gzip	260 B	260 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	386 B	386 B	✓
withRouter-HASH.js gzip	319 B	319 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	13 kB	13 kB	✓

Client Build Manifests

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
_buildManifest.js gzip	494 B	494 B	✓
Overall change	494 B	494 B	✓

Rendered Page Sizes

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
index.html gzip	539 B	539 B	✓
link.html gzip	551 B	551 B	✓
withRouter.html gzip	533 B	533 B	✓
Overall change	1.62 kB	1.62 kB	✓

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
buildDuration	8.2s	7.9s	-220ms
buildDurationCached	4s	3.8s	-184ms
nodeModulesSize	183 MB	183 MB	⚠️ +91.4 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.864	3.75	-0.11
/ avg req/sec	647.02	666.75	+19.73
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.16	2.028	-0.13
/error-in-render avg req/sec	1157.32	1232.45	+75.13

Client Bundles (main, webpack, commons)

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
675-HASH.js gzip	13.8 kB	13.8 kB	✓
770.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	50.7 kB	50.7 kB	✓
main-HASH.js gzip	34.9 kB	34.9 kB	✓
webpack-HASH.js gzip	1.64 kB	1.64 kB	✓
Overall change	101 kB	101 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
_app-HASH.js gzip	1.33 kB	1.33 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	315 B	315 B	✓
css-HASH.js gzip	331 B	331 B	✓
dynamic-HASH.js gzip	2.79 kB	2.79 kB	✓
head-HASH.js gzip	356 B	356 B	✓
hooks-HASH.js gzip	637 B	637 B	✓
image-HASH.js gzip	573 B	573 B	✓
index-HASH.js gzip	262 B	262 B	✓
link-HASH.js gzip	2.2 kB	2.2 kB	✓
routerDirect..HASH.js gzip	326 B	326 B	✓
script-HASH.js gzip	393 B	393 B	✓
withRouter-HASH.js gzip	322 B	322 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	10.1 kB	10.1 kB	✓

Client Build Manifests

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
_buildManifest.js gzip	511 B	511 B	✓
Overall change	511 B	511 B	✓

Rendered Page Sizes

	vercel/next.js canary	padmaia/next.js fix/dynamic-element-class	Change
index.html gzip	539 B	539 B	✓
link.html gzip	551 B	551 B	✓
withRouter.html gzip	532 B	532 B	✓
Overall change	1.62 kB	1.62 kB	✓

Commit: ece9d8d

ijjk · 2021-10-02T15:55:15Z

Failing test suites

Commit: ece9d8d

test/development/basic-basepath/hmr.test.ts

basic HMR > Error Recovery > should recover from errors in the render function

Expand output

● basic HMR › Error Recovery › should recover from errors in the render function

TIMED OUT: /This is the about page/

  437 |
  438 |   if (hardError) {
> 439 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content)
      |           ^
  440 |   }
  441 |   return false
  442 | }

  at Object.check (lib/next-test-utils.js:439:11)
  at Object.<anonymous> (development/basic-basepath/hmr.test.ts:458:9)

test/integration/gssp-ssr-change-reloading/test/index.test.js

GS(S)P Server-Side Change Reloading > should update page when getStaticProps is changed only
GS(S)P Server-Side Change Reloading > should show indicator when re-fetching data
GS(S)P Server-Side Change Reloading > should update page when getStaticPaths is changed only
GS(S)P Server-Side Change Reloading > should not reload page when client-side is changed too GSSP
GS(S)P Server-Side Change Reloading > should update page when getServerSideProps is changed only

Expand output

● GS(S)P Server-Side Change Reloading › should update page when getStaticProps is changed only

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  71 |       async () =>
  72 |         JSON.parse(await browser.elementByCss('#props').text()).count + '',
> 73 |       '1'
     |          ^
  74 |     )
  75 |   })
  76 |

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:73:10)

● GS(S)P Server-Side Change Reloading › should show indicator when re-fetching data

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  93 |     expect(await browser.eval(() => window.beforeChange)).toBe('hi')
  94 |     expect(await browser.eval(() => window.showedBuilder)).toBe(true)
> 95 |     page.restore()
     |                 ^
  96 |
  97 |     await check(
  98 |       async () =>

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:95:17)

● GS(S)P Server-Side Change Reloading › should update page when getStaticPaths is changed only

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  87 |
  88 |     await check(
> 89 |       async () =>
     |                  ^
  90 |         JSON.parse(await browser.elementByCss('#props').text()).count + '',
  91 |       '2'
  92 |     )

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:89:29)

● GS(S)P Server-Side Change Reloading › should not reload page when client-side is changed too GSSP

Failed to replace content.

Pattern: change me

Content: export default function Gssp(props) {
  return (
    <>
      <p id="change">changed</p>
      <p id="props">{JSON.stringify(props)}</p>
    </>
  )
}

export const getServerSideProps = ({ params }) => {
  const count = 2

  return {
    props: {
      count,
      params,
      random: Math.random(),
    },
  }
}

  467 |     } else if (typeof pattern === 'string') {
  468 |       if (!currentContent.includes(pattern)) {
> 469 |         throw new Error(
      |               ^
  470 |           `Failed to replace content.\n\nPattern: ${pattern}\n\nContent: ${currentContent}`
  471 |         )
  472 |       }

  at File.replace (lib/next-test-utils.js:469:15)
  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:163:63)

● GS(S)P Server-Side Change Reloading › should update page when getServerSideProps is changed only

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  185 |       async () =>
  186 |         JSON.parse(await browser.elementByCss('#props').text()).count + '',
> 187 |       '1'
      |          ^
  188 |     )
  189 |   })
  190 | })

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:187:10)

kdy1

LGTM. Nice work!

Include param vars in nearest scope bindings

be4d64c

padmaia requested review from huozhi, ijjk, shuding, styfle and timneutkens as code owners September 30, 2021 22:03

ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Sep 30, 2021

padmaia requested a review from kdy1 September 30, 2021 22:05

This comment has been minimized.

Sign in to view

Build next-swc binaries

63532ce

kdy1 reviewed Oct 1, 2021

View reviewed changes

packages/next/build/swc/src/styled_jsx/mod.rs Outdated Show resolved Hide resolved

packages/next/build/swc/src/styled_jsx/mod.rs Outdated Show resolved Hide resolved

packages/next/build/swc/src/styled_jsx/mod.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

PR feedback

82525a4

padmaia force-pushed the fix/dynamic-element-class branch from aa32d59 to 82525a4 Compare October 1, 2021 20:20

This comment has been minimized.

Sign in to view

padmaia and others added 2 commits October 1, 2021 20:53

Build next-swc binaries

e31be88

Merge branch 'canary' into fix/dynamic-element-class

7755e2d

This comment has been minimized.

Sign in to view

ijjk approved these changes Oct 2, 2021

View reviewed changes

Merge branch 'canary' into fix/dynamic-element-class

ece9d8d

kdy1 approved these changes Oct 2, 2021

View reviewed changes

timneutkens merged commit 260e0e4 into vercel:canary Oct 3, 2021

ijjk added the hacktoberfest-accepted label Oct 4, 2021

vercel locked as resolved and limited conversation to collaborators Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include param vars in nearest scope bindings #29536

Include param vars in nearest scope bindings #29536

padmaia commented Sep 30, 2021

This comment has been minimized.

This comment has been minimized.

kdy1 left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Oct 2, 2021

ijjk commented Oct 2, 2021 •

edited

Loading

kdy1 left a comment

Include param vars in nearest scope bindings #29536

Include param vars in nearest scope bindings #29536

Conversation

padmaia commented Sep 30, 2021

This comment has been minimized.

This comment has been minimized.

kdy1 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Oct 2, 2021

Stats from current PR

ijjk commented Oct 2, 2021 • edited Loading

Failing test suites

kdy1 left a comment

Choose a reason for hiding this comment

ijjk commented Oct 2, 2021 •

edited

Loading