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

doc: improved fetch docs #57296

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

doc: improved fetch docs #57296

wants to merge 2 commits into from

Conversation

lifeisfoo
Copy link

Updated fetch documentation:

  • added a fetch example
  • explained its connection with undici
  • explained how to get the current undici version
  • explained how to use a custom dispatcher
  • added link to related HTTP classes

Refs:

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 3, 2025
@panva
Copy link
Member

panva commented Mar 3, 2025

cc @nodejs/undici

@KhafraDev
Copy link
Member

I am personally a -1: the basic example has bad practices (not consuming the body if (!res.ok)); Dispatcher/Agent aren't exposed in node (so why document them here?); FormData, Response, Request, and Headers are already documented.

@lifeisfoo
Copy link
Author

lifeisfoo commented Mar 3, 2025

@KhafraDev I took the basic example from Node.js 18 release notes so I thought it was ok, but it can be improved.

Dispatcher/Agent aren't exposed in node (so why document them here?);

As commented here and answered here:

You can consider it (globalThis[Symbol.for('undici.globalDispatcher.1')] ndr) part of the public API, we should probably document it.

Moreover, there is also 3K views question on Stackoverflow.

FormData, Response, Request, and Headers are already documented

Yes, but these classes are relevant to fetch.

@gdorsi
Copy link

gdorsi commented Mar 4, 2025

Thanks @lifeisfoo for the PR and @KhafraDev for the review!

Jumping here because the discussion in this PR piqued my curiosity.

the basic example has bad practices (not consuming the body if (!res.ok));

Curious, why it's a bad practice and what would be the good practice?

@gdorsi
Copy link

gdorsi commented Mar 4, 2025

Dispatcher/Agent aren't exposed in node (so why document them here?);

Setting up a custom Agent is quite a common use case and we can't just redirect to the Undici documentation, because as you said, Dispatcher/Agent aren't exposed in node.

What would be your suggestion? Shall we suggest to use undici direclty when setting up a custom Agent is needed?

@lifeisfoo
Copy link
Author

The expanded fetch documentation could also contain information about the upcoming HTTP_PROXY env var support and the support for SOCKS proxies through undici.

Incidentally, I created this PR after struggling to work with a SOCKS proxy in Node.js, where the dispatcher override function is the key to using it.

`globalThis[Symbol.for('undici.globalDispatcher.1')] = socksDispatcher;`

From my point of view, documentation should aim to save developers hours by giving them all the information they need without having to look at the source code or perform searches on other websites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants