-
Notifications
You must be signed in to change notification settings - Fork 10
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: support global "_Headers" in Undici 5.26.3 #73
Conversation
Node.js's update to Undici 5.26.3 caused its `Headers` class to be called `_Headers`: https://github.com/nodejs/node/pull/50153/files#diff-f516ab824a7722da938a4c7c851520d39731ddeb4f7198dff4e932c5d4f8fdf7
Alternatively, would it be better to check /**
* @note Cannot necessarily check if the `init` is an instance of the
* `Headers` because that class may not be defined in Node or jsdom.
*/
if (
['Headers', 'HeadersPolyfill'].includes(init?.constructor.name) ||
init instanceof Headers ||
(typeof globalThis.Headers === 'function' && init instanceof globalThis.Headers)
) { |
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.
Thanks for fixing this, @joshkel!
src/Headers.ts
Outdated
@@ -25,7 +25,7 @@ export class Headers { | |||
* because that class is only defined in the browser. | |||
*/ | |||
if ( | |||
['Headers', 'HeadersPolyfill'].includes(init?.constructor.name) || | |||
['Headers', 'HeadersPolyfill', '_Headers'].includes(init?.constructor.name) || |
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.
We should also add a test for this, implicitly. Bumping the used Node.js version in the CI would be enough. I recommend adding a matrix of Node.js versions so we test both older versions and the recent ones that introduce this change.
Let me know if you need help with this.
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.
@kettanaito Done, I think. I have no real experience working with GitHub Actions, so I don't know if my approach is will work and is the best way of doing it.
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.
You did it right 👌
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 fantastic. Thank you, @joshkel!
Welcome to contributors, @joshkel 🎉 |
Released: v4.0.2 🎉This has been released in v4.0.2! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Node.js's update to Undici 5.26.3 caused its
Headers
class to be called_Headers
:https://github.com/nodejs/node/pull/50153/files#diff-f516ab824a7722da938a4c7c851520d39731ddeb4f7198dff4e932c5d4f8fdf7
Fixes #72.