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.

Sign up for GitHub

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

Jump to bottom

Fix global process testing for the process polyfill #33220

Merged
merged 6 commits into from Jan 13, 2022
Merged

Fix global process testing for the process polyfill #33220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
81a3395
Fix global process testing for the process polyfill
Schniz Jan 12, 2022
4822dda
Ensure a failing test
Schniz Jan 12, 2022
17bf828
Fix page count
Schniz Jan 12, 2022
e1a8107
Fix the underlying bug
Schniz Jan 12, 2022
ecb2b68
Check that process.env is not null
Schniz Jan 13, 2022
0956aa7
Merge branch 'canary' into fix-global-process-testing
kodiakhq[bot] Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/next/build/polyfills/process.js
View file
Open in desktop
@@ -1 +1,4 @@
module.exports = global.process || require('../../compiled/process')
module.exports =
global.process?.env && typeof global.process?.env === 'object'
? global.process
: require('../../compiled/process')
Copy link
Member

@styfle styfle Jan 12, 2022 •
edited

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
}

Copy link
Member

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.

Copy link
Contributor Author

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.

7 changes: 7 additions & 0 deletions test/integration/polyfills/pages/process.js
View file
Open in desktop
@@ -0,0 +1,7 @@
export default function Page() {
return (
<div id="process">
Hello, {process.env.NEXT_PUBLIC_VISITOR ?? 'stranger'}
</div>
)
}
13 changes: 11 additions & 2 deletions test/integration/polyfills/test/index.test.js
View file
Open in desktop
Expand Up @@ -36,9 +36,18 @@ describe('Polyfills', () => {
await browser.close()
})

it('should allow using process.env when there is an element with `id` of `process`', async () => {
const browser = await webdriver(appPort, '/process')
const text = await browser.elementByCss('#process').text()

expect(text).toBe('Hello, stranger')

await browser.close()
})

it('should contain generated page count in output', async () => {
expect(output).toContain('Generating static pages (0/4)')
expect(output).toContain('Generating static pages (4/4)')
expect(output).toContain('Generating static pages (0/5)')
expect(output).toContain('Generating static pages (5/5)')
// we should only have 1 segment and the initial message logged out
expect(output.match(/Generating static pages/g).length).toBe(5)
})
Expand Down