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: require toUSVString from fetch utils #1526
Conversation
Since `util.toUSVString` doesn't exist at node12, in this way the fallback implementation will be used.
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.
why this change?
mainly to make |
I'm a bit lost. None of fetch should run on v12. |
1 similar comment
I'm a bit lost. None of fetch should run on v12. |
Presently, undici's package.json specifies 12.18 as the minimum version: Lines 99 to 101 in 5ca25c2
In reality, it requires >16.9. Merging this PR wouldn't change that, since undici also uses Line 385 in 5ca25c2
|
Can you articulate? We have our suite testing everything on 12, minus fetch - which is v16+ and clearly documented as such. I'd be happy to work on whatever incompatibility there is as long as I get a reproduction. |
I'd be happy to land this as long as we get a test that is failing on Node v12 - so we know we won't regress. |
Sure — using Node 16.7: const { Response } = require('undici');
new Response('ok');
/*
Uncaught TypeError: webidl.converters.USVString is not a function
at Object.webidl.converters.XMLHttpRequestBodyInit (/[dir]/node_modules/undici/lib/fetch/response.js:516:30)
at Object.webidl.converters.BodyInit (/[dir]/node_modules/undici/lib/fetch/response.js:554:28)
at new Response (/[dir]/node_modules/undici/lib/fetch/response.js:138:32)
*/ Using Node 16.8: const { Response } = require('undici');
new Response('ok');
/*
Uncaught TypeError: Object.hasOwn is not a function
at Object.ResponseInit (/[dir]/node_modules/undici/lib/fetch/webidl.js:327:33)
at new Response (/[dir]/node_modules/undici/lib/fetch/response.js:141:30)
*/ |
If I understand it correctly: the whole of @ronag do we want to support v16.7 at all? Our tests are currently failing there. The issue at the top was mentioning Node.js v12, which is what took me off guard. |
Support should be easy enough, I don't see a reason not to. I'll work on a PR to support v16.5+ again. |
Just add 16.5.0 to CI so we don't regress again. |
closing since #1534 addressed it |
Since
util.toUSVString
doesn't exist at node12, in this way the fallback implementation will be used.