Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: implement net.fetch #36733
feat: implement net.fetch #36733
Changes from 14 commits
bc75a67
c88806a
41b2210
befbe6e
64ad0ca
20ac105
b43ace8
6057958
c0f23c3
11de4bc
681597f
2104b3d
6bdc04d
d7d044b
fcfc826
e484046
7ace33f
dff55f0
a9c965d
8234acf
2f4909b
dbf6b90
aaf9379
05efb81
4e0d1f1
b4b4e81
a1764ac
58c9dc2
0491ab4
79afe45
3829c76
4b233bd
c071c8b
cef954e
7c26527
f87690d
2e95959
042dc49
d97e86f
3d1cc34
ca6acfc
f9a7237
fe5a2ad
12cdf0a
714fc66
94330f4
408ef5b
3b1e86b
45c15d9
ff0a824
973b8ce
5c399a3
4a11e4e
bfb16b2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think it'd be useful to include a short explainer on why this might be useful. I have a few ideas, but I imagine many readers would not be sure when to use 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.
That's a good idea. I'm not sure exactly what I'd suggest though, other than "this is a nicer API than net.request" :P
My main impetus for adding this is because it fits conveniently into later plans I have for
protocol.handle
. What do you think some good things to suggest here would be?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.
Using Chrome's network stack can be better for handling network proxies, right? Maybe something about that. Also, would it allow requests made using this API to be intercepted with webRequest?
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.
Ah, you mean to contrast it with Node's net stack, rather than to contrast it with
net.request
. That makes sense, though sincenet.fetch
andnet.request
use the same network stack, I think such a distinction belongs at the top of net.md, as opposed to here in thefetch
documentation (though we should link it here).Since that would be additional documentation for both
net.request
andnet.fetch
, I'd like to do that in a followup PR.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.
Are the
RequestInfo
andRequestInit
types coming from Node.js ?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.
yep.
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.
actually, it looks like these types are coming from
lib.dom.d.ts
in Typescript. @types/node doesn't export them, though undici does define them: https://github.com/nodejs/undici/blob/2b260c997ad4efe4ed2064b264b4b546a59e7a67/types/fetch.d.ts#L103This turns out to cause some minor issues (e.g. TypeScript getting confused between the Web's
ReadableStream
and Node's Web-compatibleReadableStream
) but on the whole the types should be pretty similar, so not too big a deal I think, at least until Node sorts out what it's doing w.r.t. fetch &c.