-
Notifications
You must be signed in to change notification settings - Fork 26.7k
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
Fix global process testing for the process polyfill #33220
Conversation
When there is a DOM element with id of `process`, the DOM marks it as a global, so `window.process` would exist. We should check for `process.env` to make sure it is available too.
Failing test suitesCommit: 4822dda test/integration/polyfills/test/index.test.js
Expand output● Polyfills › should contain generated page count in output
|
This comment has been minimized.
This comment has been minimized.
but I can't make the test fail on this repo. However, it does happen when I create a new repo with the same code.
This comment has been minimized.
This comment has been minimized.
module.exports = | ||
typeof global.process?.env === 'object' | ||
? global.process | ||
: require('../../compiled/process') |
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 this needs to mutate global.process
if it exists because its referencing the DOM element so you just need to add process.env
to it too. Something like this:
const polyfill = require('../../compiled/process')
module.exports = {
...global.process,
...polyfill
}
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 don't think that's a good idea. I think we should not use the global process
object when it's not a "process" thing.
Otherwise you get random properties from the DOM objects into the process.
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.
Sounds like a production decision, I'm fine either way. I'm more biased towards the current implementation, because the following code:
// pages/my-page.js
import {useEffect} from 'react'
export default function MyComponent() {
useEffect(() => {
console.log(window.process.dataset, process.dataset)
});
return <div id="preview" data-hey="ho">Hello, {process.env.NEXT_PUBLIC_VISITOR}</div>
}
renders Hello, stranger
and console logs DOMStringMap { hey: "ho" }, undefined
. So we don't "leak" DOM data into what is treated as Node.js API when server rendering. The window.
usage is explicit.
Co-authored-by: JJ Kasper <jj@jjsweb.site>
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.
This looks good to me as it handles the cases we identified specifically where an element with id
of process
exists we will use the polyfill since the element won't have a proper env
field.
This comment has been minimized.
This comment has been minimized.
Stats from current PRDefault Build (Decrease detected ✓)General Overall increase
|
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
buildDuration | 14.7s | 15.3s | |
buildDurationCached | 3.2s | 3.1s | -130ms |
nodeModulesSize | 354 MB | 354 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 2.873 | 2.936 | |
/ avg req/sec | 870.04 | 851.45 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.306 | 1.312 | |
/error-in-render avg req/sec | 1914.06 | 1905.27 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.2 kB | 42.2 kB | ✓ |
main-HASH.js gzip | 27.2 kB | 27.2 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71 kB | 71 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.37 kB | 1.37 kB | ✓ |
_error-HASH.js gzip | 194 B | 194 B | ✓ |
amp-HASH.js gzip | 312 B | 312 B | ✓ |
css-HASH.js gzip | 326 B | 326 B | ✓ |
dynamic-HASH.js gzip | 2.37 kB | 2.37 kB | ✓ |
head-HASH.js gzip | 350 B | 350 B | ✓ |
hooks-HASH.js gzip | 919 B | 919 B | ✓ |
image-HASH.js gzip | 4.7 kB | 4.7 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 2.13 kB | 2.13 kB | ✓ |
routerDirect..HASH.js gzip | 321 B | 321 B | ✓ |
script-HASH.js gzip | 383 B | 383 B | ✓ |
withRouter-HASH.js gzip | 318 B | 318 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 14.1 kB | 14.1 kB | ✓ |
Client Build Manifests
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 459 B | ✓ |
Overall change | 459 B | 459 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
index.html gzip | 531 B | 531 B | ✓ |
link.html gzip | 545 B | 545 B | ✓ |
withRouter.html gzip | 526 B | 526 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
buildDuration | 15.8s | 15.2s | -572ms |
buildDurationCached | 3.1s | 3.1s | |
nodeModulesSize | 354 MB | 354 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 2.902 | 3.509 | |
/ avg req/sec | 861.5 | 712.47 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.365 | 1.325 | -0.04 |
/error-in-render avg req/sec | 1832.09 | 1887.23 | +55.14 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.3 kB | 42.3 kB | ✓ |
main-HASH.js gzip | 27.3 kB | 27.3 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71.2 kB | 71.2 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
_error-HASH.js gzip | 180 B | 180 B | ✓ |
amp-HASH.js gzip | 305 B | 305 B | ✓ |
css-HASH.js gzip | 321 B | 321 B | ✓ |
dynamic-HASH.js gzip | 2.36 kB | 2.36 kB | ✓ |
head-HASH.js gzip | 342 B | 342 B | ✓ |
hooks-HASH.js gzip | 906 B | 906 B | ✓ |
image-HASH.js gzip | 4.72 kB | 4.72 kB | ✓ |
index-HASH.js gzip | 256 B | 256 B | ✓ |
link-HASH.js gzip | 2.19 kB | 2.19 kB | ✓ |
routerDirect..HASH.js gzip | 314 B | 314 B | ✓ |
script-HASH.js gzip | 375 B | 375 B | ✓ |
withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 14 kB | 14 kB | ✓ |
Client Build Manifests
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
_buildManifest.js gzip | 458 B | 458 B | ✓ |
Overall change | 458 B | 458 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | Schniz/next.js fix-global-process-testing | Change | |
---|---|---|---|
index.html gzip | 530 B | 530 B | ✓ |
link.html gzip | 544 B | 544 B | ✓ |
withRouter.html gzip | 526 B | 526 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
When there is a DOM element with id of
process
, the DOM marks it as a global, sowindow.process
would exist. We should check forprocess.env
to make sure it is available too.