-
Notifications
You must be signed in to change notification settings - Fork 530
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(fetch): re-add support for node v16.8.0+ #1534
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1534 +/- ##
=======================================
Coverage 94.90% 94.90%
=======================================
Files 50 50
Lines 4608 4609 +1
=======================================
+ Hits 4373 4374 +1
Misses 235 235
Continue to review full report at Codecov.
|
@ronag - isDisturbed doesn't seem to work on node v16.5 ( Line 295 in c1a0490
edit: minimal repro (this should throw a TypeError) import { fetch } from './index.js'
const controller = new AbortController()
const res = await fetch('https://www.google.com', {
signal: controller.signal
})
controller.abort()
await res.text() |
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.
lgtm
I don't think we can do without |
The code is mostly similar to core other than core checks if a symbol is true on the object and does a Lines 295 to 304 in c1a0490
|
That's why we added it in core. You can't implement fetch() with only the public API from WHATWG streams. That Symbol expose some internal state. |
There were a few existing issues with node v16.5 that make fetch largely incompatible.
edit: node-fetch test is still failing. Didn't experience this issue locally... |
Why did you close this? |
Didn't realize lol. Need to bump the min version for fetch to 16.8. |
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.
lgtm
Fetch support incorrectly claimed it would work on node >= v16.5.0.
URL.createObjectURL
).stream.isDisturbed
).See #1526 (specifically this comment) for more context.
Also adds node v16.8.0 to the test matrix.