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.

Sign up for GitHub

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

Jump to bottom

Fetch API implementation for Node #154

Merged
merged 4 commits into from
Jan 30, 2023
Merged

Fetch API implementation for Node #154

merged 4 commits into from
Jan 30, 2023

Conversation

ardatan
Copy link
Owner

@ardatan ardatan commented Oct 4, 2022 •
edited
Loading

Will also close #197

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022 •
edited
Loading

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@whatwg-node/fetch 0.6.3-alpha-20230126122900-db01245 npm ↗︎ unpkg ↗︎
@whatwg-node/node-fetch 0.0.1-alpha-20230126122900-db01245 npm ↗︎ unpkg ↗︎
@whatwg-node/router 0.1.8-alpha-20230126122900-db01245 npm ↗︎ unpkg ↗︎
@whatwg-node/server 0.5.9-alpha-20230126122900-db01245 npm ↗︎ unpkg ↗︎

@ardatan ardatan marked this pull request as ready for review December 25, 2022 15:53
@ardatan ardatan mentioned this pull request Dec 30, 2022
Merged
@ardatan ardatan force-pushed the new-node-fetch branch 2 times, most recently from 69140df to 02b2895 Compare January 2, 2023 10:22
enisdenjo
enisdenjo approved these changes Jan 12, 2023
View reviewed changes
Copy link
Collaborator

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

This looks awesome, great job! I haven't reviewed the in great detail, am trusting your judgement there. 😄

dotansimha
dotansimha reviewed Jan 17, 2023
View reviewed changes
packages/node-fetch/src/Response.ts Outdated
});
}

static json(data: any, init: RequestInit = {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about adding the generic json<TResponse> here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

But the return type doesn't have a generic. Would it be really useful?

dotansimha
dotansimha approved these changes Jan 17, 2023
View reviewed changes
Copy link
Collaborator

@dotansimha dotansimha left a comment

Choose a reason for hiding this comment

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

@ardatan LGTM. I would just say that maybe we can improve the TypeScript interfaces and make them more TS-friendly? (when it comes to generics, return types and so on)

@dotansimha
Copy link
Collaborator

dotansimha commented Jan 17, 2023 •
edited
Loading

Also, @ardatan , do you think it's possible to add a CI check that runs other repos tests, with our fetch? Maybe even run node-fetch or node's test suite with ours? (just to make sure we are always compatible)

@ardatan
Copy link
Owner Author

ardatan commented Jan 17, 2023

@dotansimha Actually TS interfaces are not used directly by the consumer. It is imported by @whatwg-node/fetch as a ponyfill just like undici or node-fetch, and TypeScript's native Fetch API typings are used by @whatwg-node/fetch in order to be compatible with other fetch impl.
But we can use this package's typings directly instead. For now, my main goal was to implement existing native interfaces.
Let's make it after if you think it is not that important for now.
About tests, not sure how it is going to look like. I think we can gradually improve tests on our own.

@dotansimha
Copy link
Collaborator

Got it, thanks!

About tests, not sure how it is going to look like. I think we can gradually improve tests on our own.

Regarding this, I have to say that this is kinda a big concern, because we are going to introduce a replacement that should work the same way, and we don't have a way to test edge-cases at the moment

kamilkisiela
kamilkisiela approved these changes Jan 17, 2023
View reviewed changes
Copy link
Collaborator

@kamilkisiela kamilkisiela left a comment

Choose a reason for hiding this comment

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

LGTM I guess

YassinEldeeb
YassinEldeeb approved these changes Jan 17, 2023
View reviewed changes
Copy link

@YassinEldeeb YassinEldeeb left a comment

Choose a reason for hiding this comment

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

looks good

saihaj
saihaj approved these changes Jan 18, 2023
View reviewed changes
Copy link
Collaborator

@saihaj saihaj left a comment

Choose a reason for hiding this comment

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

we can add more tests in future PR. Excited to see use this!

@ardatan ardatan force-pushed the new-node-fetch branch 2 times, most recently from 03f65c9 to 900e6b6 Compare January 30, 2023 16:21
@ardatan ardatan merged commit 9f4fe48 into master Jan 30, 2023
@ardatan ardatan deleted the new-node-fetch branch January 30, 2023 16:24
@github-actions github-actions bot mentioned this pull request Jan 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YassinEldeeb YassinEldeeb YassinEldeeb approved these changes

@saihaj saihaj saihaj approved these changes

@enisdenjo enisdenjo enisdenjo approved these changes

@kamilkisiela kamilkisiela kamilkisiela approved these changes

@dotansimha dotansimha dotansimha approved these changes

@n1ru4l n1ru4l Awaiting requested review from n1ru4l

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

@whatwg-node/[email protected]" has incorrect peer dependency "@types/node@^18.0.6"
6 participants
@ardatan @dotansimha @kamilkisiela @enisdenjo @saihaj @YassinEldeeb