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

misc: drop node 16 support #15290

Merged
merged 11 commits into from
Jul 27, 2023
Merged

misc: drop node 16 support #15290

merged 11 commits into from
Jul 27, 2023

Conversation

adamraine
Copy link
Member

@adamraine adamraine requested a review from a team as a code owner July 19, 2023 22:21
@adamraine adamraine requested review from connorjclark and removed request for a team July 19, 2023 22:21
@adamraine adamraine mentioned this pull request Jul 19, 2023
20 tasks
@@ -82,7 +82,7 @@ async function runBundledLighthouse(url, config, testRunnerOptions) {
const logLevel = testRunnerOptions.isDebug ? 'verbose' : 'info';

// Puppeteer is not included in the bundle, we must create the page here.
const browser = await puppeteer.connect({browserURL: `http://localhost:${port}`});
const browser = await puppeteer.connect({browserURL: `http://127.0.0.1:${port}`});
Copy link
Member Author

@adamraine adamraine Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node http/fetch no longer autoresolves localhost to the ipv4 address nodejs/node#40702

I guess it does in Node 20 though so this is only an issue for Node 18

@@ -424,7 +424,7 @@ describe('.resolveGathererToDefn', () => {
it('throws but not for missing gatherer when it has a node dependency error', async () => {
const resultPromise =
resolveGathererToDefn('../fixtures/invalid-gatherers/require-error.js', [], moduleDir);
await expect(resultPromise).rejects.toThrow(/Cannot find module/);
await expect(resultPromise).rejects.toThrow(/no such file or directory/);
Copy link
Member Author

@adamraine adamraine Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Cannot find module" text came from an error constructed by quibble. Updates in v0.7.0 of quibble appear to surface the actual node error instead. Good change IMO.

'Content-Security-Policy': `default-src 'none';`,
});
response.statusCode = 200;
response.setHeader('Content-Security-Policy', `default-src 'none';`);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm can't find any info in the docs, but for some reason response.writeHead will end the response in Node 20 and send it to the user. We want to be able to amend the headers and status code later in execution (e.g. if there was a redirect) so we should use setHeader and statusCode = now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.json Outdated
@@ -10,7 +10,7 @@
"smokehouse": "./cli/test/smokehouse/frontends/smokehouse-bin.js"
},
"engines": {
"node": ">=16.16"
"node": ">=18"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try on 18.0.0?

Copy link
Member Author

@adamraine adamraine Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I got some failures locally on 18.0.0 so I'll add a minor version

@adamraine adamraine merged commit 86a385b into main Jul 27, 2023
24 checks passed
@adamraine adamraine deleted the drop-node-16 branch July 27, 2023 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants