Skip to content
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

Merged
merged 1 commit into from
Aug 25, 2021
Merged

feat: fetch types #997

merged 1 commit into from
Aug 25, 2021

Conversation

wojpawlik
Copy link
Contributor

This relates to...

fixes #953

Status

KEY: S = Skipped, x = complete

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2021

Codecov Report

Merging #997 (de02ade) into main (f2b0b67) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2b0b67...de02ade. Read the comment docs.

@ronag ronag requested a review from mcollina August 24, 2021 09:36
Copy link
Member

@szmarczak szmarczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@mcollina mcollina left a 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?

@wojpawlik
Copy link
Contributor Author

wojpawlik commented Aug 24, 2021

No. I've never tested types before. I have no idea what exactly needs to he tested.

@szmarczak
Copy link
Member

szmarczak commented Aug 24, 2021

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 tsd assertions.

@Ethan-Arrowood
Copy link
Collaborator

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 fetch are available in lib.dom (https://github.com/microsoft/TypeScript/blob/cec2fda9a53620dc545a2c4d7b0156446ab145b4/lib/lib.dom.d.ts#L18378) here is a link to the exact fetch definition

@wojpawlik wojpawlik marked this pull request as draft August 24, 2021 18:36
@wojpawlik
Copy link
Contributor Author

I was improving the types by comparing them on MDN. So now I'm working on adding FormData types as well, because it's valid BodyInit.

readonly bodyUsed: boolean

readonly arrayBuffer: () => Promise<Buffer>
readonly json: () => Promise<unknown>
Copy link
Member

@KhafraDev KhafraDev Aug 25, 2021

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. 👍

Copy link
Contributor Author

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

@wojpawlik wojpawlik marked this pull request as ready for review August 25, 2021 10:03
@@ -0,0 +1,97 @@
// based on https://unpkg.com/browse/formdata-node@4.0.1/@type/FormData.d.ts (MIT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ronag ronag merged commit 5425b77 into nodejs:main Aug 25, 2021
@szmarczak
Copy link
Member

Big thanks @wojpawlik 🙌🏼

KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Definition of fetch missing from types
7 participants