-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Fixes beforeInteractive
scripts failing in custom document
#37000
Conversation
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
buildDuration | 18.7s | 18.3s | -420ms |
buildDurationCached | 7.4s | 7.5s | |
nodeModulesSize | 478 MB | 478 MB |
Page Load Tests Overall increase ✓
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 4.586 | 4.624 | |
/ avg req/sec | 545.16 | 540.71 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.585 | 1.575 | -0.01 |
/error-in-render avg req/sec | 1577.76 | 1587.55 | +9.79 |
Client Bundles (main, webpack)
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
925.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42 kB | 42 kB | ✓ |
main-HASH.js gzip | 28.8 kB | 28.8 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 72.4 kB | 72.4 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.36 kB | 1.36 kB | ✓ |
_error-HASH.js gzip | 193 B | 193 B | ✓ |
amp-HASH.js gzip | 308 B | 308 B | ✓ |
css-HASH.js gzip | 327 B | 327 B | ✓ |
dynamic-HASH.js gzip | 2.71 kB | 2.71 kB | ✓ |
head-HASH.js gzip | 359 B | 359 B | ✓ |
hooks-HASH.js gzip | 920 B | 920 B | ✓ |
image-HASH.js gzip | 5.72 kB | 5.72 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 2.65 kB | 2.65 kB | ✓ |
routerDirect..HASH.js gzip | 320 B | 320 B | ✓ |
script-HASH.js gzip | 391 B | 391 B | ✓ |
withRouter-HASH.js gzip | 318 B | 318 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 16 kB | 16 kB | ✓ |
Client Build Manifests
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
_buildManifest.js gzip | 460 B | 460 B | ✓ |
Overall change | 460 B | 460 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
index.html gzip | 531 B | 531 B | ✓ |
link.html gzip | 545 B | 545 B | ✓ |
withRouter.html gzip | 528 B | 528 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
buildDuration | 21.1s | 21s | -72ms |
buildDurationCached | 7.6s | 7.5s | -65ms |
nodeModulesSize | 478 MB | 478 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 4.597 | 4.725 | |
/ avg req/sec | 543.82 | 529.07 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.585 | 1.655 | |
/error-in-render avg req/sec | 1577.78 | 1510.23 |
Client Bundles (main, webpack)
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
925.HASH.js gzip | 178 B | 178 B | ✓ |
framework-HASH.js gzip | 42.7 kB | 42.7 kB | ✓ |
main-HASH.js gzip | 29.2 kB | 29.2 kB | ✓ |
webpack-HASH.js gzip | 1.45 kB | 1.45 kB | ✓ |
Overall change | 73.5 kB | 73.5 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
_error-HASH.js gzip | 179 B | 179 B | ✓ |
amp-HASH.js gzip | 311 B | 311 B | ✓ |
css-HASH.js gzip | 324 B | 324 B | ✓ |
dynamic-HASH.js gzip | 2.89 kB | 2.89 kB | ✓ |
head-HASH.js gzip | 357 B | 357 B | ✓ |
hooks-HASH.js gzip | 920 B | 920 B | ✓ |
image-HASH.js gzip | 5.81 kB | 5.81 kB | ✓ |
index-HASH.js gzip | 261 B | 261 B | ✓ |
link-HASH.js gzip | 2.78 kB | 2.78 kB | ✓ |
routerDirect..HASH.js gzip | 322 B | 322 B | ✓ |
script-HASH.js gzip | 392 B | 392 B | ✓ |
withRouter-HASH.js gzip | 317 B | 317 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 16.3 kB | 16.3 kB | ✓ |
Client Build Manifests
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
_buildManifest.js gzip | 457 B | 457 B | ✓ |
Overall change | 457 B | 457 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | housseindjirdeh/next.js bug/#36997 | Change | |
---|---|---|---|
index.html gzip | 531 B | 531 B | ✓ |
link.html gzip | 546 B | 546 B | ✓ |
withRouter.html gzip | 528 B | 528 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
9813e11
to
e6f2cc5
Compare
This was passing. Our tests would have failed otherwise, since none of the Script tags would have rendered. Not sure what changed |
packages/next/pages/_document.tsx
Outdated
@@ -605,7 +605,7 @@ export class Head extends Component< | |||
const filteredChildren: ReactNode[] = [] | |||
|
|||
React.Children.forEach(children, (child: any) => { | |||
if (child.type === Script) { | |||
if (typeof child.type === 'function' && child.type.name === 'Script') { |
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.
I think the previous check of child.type === Script
was correct as that ensures it is the next/script
component and not a custom component named Script
The issue seemed to be from the import for next/script
in _document
being local ../client/script
and the other being non-local causing different instances to be loaded. The tests passed because the monorepo version of Next.js is being used and webpack treated both instances the same.
We should be able to add a regression test for this by adding it to an isolated test (test/e2e
or test/production
)
We can also fix the different instances being loaded by updating this line:
next.js/packages/next/build/webpack-config.ts
Line 819 in 4a86a8f
/^(?:private-next-pages\/|next\/(?:dist\/pages\/|(?:app|document|link|image|constants|dynamic)$)|string-hash$)/ |
to: /^(?:private-next-pages\/|next\/(?:dist\/pages\/|(?:app|document|link|image|constants|dynamic|script)$)|string-hash$)/
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.
Thanks so much clarifying, I wasn't aware that different instances of the same import could have been the issue.
Just fixed it and added a test 👍
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.
Thanks!
beforeInteractive
not working in_document.js
#36997dangerouslySetInnerHTML
andbeforeInteractive
strategy in Script component #31275@janicklas-ralph Any idea why tests were passing while this check was failing? Can we add a stronger test for this?