-
Notifications
You must be signed in to change notification settings - Fork 516
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
feat: fetch
types
#997
feat: fetch
types
#997
Conversation
Codecov Report
@@ Coverage Diff @@
## main #997 +/- ##
=======================================
Coverage 94.03% 94.03%
=======================================
Files 37 37
Lines 3585 3585
=======================================
Hits 3371 3371
Misses 214 214 Continue to review full report at Codecov.
|
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.
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.
Could you add tests?
No. I've never tested types before. I have no idea what exactly needs to he tested. |
You can find the current ts tests here: https://github.com/nodejs/undici/tree/main/test/types The tests are just basic code as you would've used it in a real app but with the difference that you check the types with |
Since @ronag did a lot of work to make fetch spec compat here lets make sure we are not missing anything. The official TS types for |
I was improving the types by comparing them on MDN. So now I'm working on adding |
readonly bodyUsed: boolean | ||
|
||
readonly arrayBuffer: () => Promise<Buffer> | ||
readonly json: () => Promise<unknown> |
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.
I just want to point out that this will break type compatibility with other packages & the native types.
const r = await fetch('https://some-json-api.com/');
const b = await r.json();
// in node-fetch, native types, etc (without casts)
b.something.somethingelse.anotherthing
// with this
b.something
^ 'cannot read property "something" of unknown'
unknown
is a much better type than any
, but it adds an extra step for those transitioning from another package. Not sure if it really matters to you guys but I just wanted to point it out. 👍
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.
node-fetch@3.0.0-beta.10
uses Promise<unknown>
as well, and switching to Promise<any>
wouldn't be breaking
@@ -0,0 +1,97 @@ | |||
// based on https://unpkg.com/browse/formdata-node@4.0.1/@type/FormData.d.ts (MIT) |
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.
// based on https://unpkg.com/browse/formdata-node@4.0.1/@type/FormData.d.ts (MIT) | |
// based on https://github.com/octet-stream/form-data (MIT) |
fixes #953
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
Big thanks @wojpawlik 🙌🏼 |
This relates to...
fixes #953
Status
KEY: S = Skipped, x = complete