Skip to content

Conversation

@adamraine
Copy link
Contributor

@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

// 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
Contributor 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

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
Contributor 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
Contributor 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
},
"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
Contributor 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
@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