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

Document on how to use the new fetch API equivalent to http agents #613

Closed
oscard0m opened this issue Jul 9, 2023 · 6 comments · Fixed by #614
Closed

Document on how to use the new fetch API equivalent to http agents #613

oscard0m opened this issue Jul 9, 2023 · 6 comments · Fixed by #614
Assignees
Labels
released Type: Documentation Improvements or additions to documentation

Comments

@oscard0m
Copy link
Member

oscard0m commented Jul 9, 2023

Currently we have a PR in the works, and discussions happening over in the @octokit/request repo: #599

We still need documentation on how to use the new fetch API equivalent to http agents.
If you'd like you can take that up

Originally posted by @wolfy1339 in octokit/octokit.js#2484 (reply in thread)

@oscard0m oscard0m added the Type: Documentation Improvements or additions to documentation label Jul 9, 2023
@github-project-automation github-project-automation bot moved this to 🆕 Triage in 🧰 Octokit Active Jul 9, 2023
@oscard0m oscard0m self-assigned this Jul 9, 2023
@oscard0m
Copy link
Member Author

oscard0m commented Jul 9, 2023

I probably will need some guidance here because I don't have experience with http.Agent. I created a CodeSandbox code example where we can comment what's the direction to take: https://codesandbox.io/p/sandbox/nifty-stitch-wdlwlf

  1. I need to figure out how to test if my overridden fetch is being called and its dispatcher is working.
  2. I'm not sure if this is an option we want to document: Is it possible to create a custom `fetch` with the `dispatcher` option preset nodejs/undici#2167 (reply in thread). To me sounds like it would override all the occurrences of fetch which is something users may don't want. Is this true?

Node.js GitHub Discussion for more context: nodejs/undici#2167

Tagging @gr2m because I think you have more context on this.

@wolfy1339
Copy link
Member

wolfy1339 commented Jul 9, 2023

  1. I need to figure out how to test if my overridden fetch is being called and its dispatcher is working.

We use that option all the time in the tests, so it works for sure.

You can't use http.Agent anymore. you need to use undici.Agent()

I'm not sure we really want to document the dispatcher option, as it's not standard and only exists for undici...
We recently removed non-standard options that were node-fetch only

@wolfy1339 wolfy1339 moved this from 🆕 Triage to 🏗 In progress in 🧰 Octokit Active Jul 9, 2023
@oscard0m
Copy link
Member Author

oscard0m commented Jul 9, 2023

We use that option all the time in the tests, so it works for sure.

👍🏽

You can't use http.Agent anymore. you need to use undici.Agent()

👍🏽

I'm not sure we really want to document the dispatcher option, as it's not standard and only exists for undici... We recently removed non-standard options that were node-fetch only

This is what @gr2m posted:

Add documentation how to create a custom fetch function with a dispatcher preset (it's easy, nodejs/undici#2167)

I updated the CodeSandbox example with an example with undici + dispatcher but let me know if that would be what we want to document or just link to undici's docs and that's it.

@gr2m
Copy link
Contributor

gr2m commented Jul 9, 2023

Codesandbox example looks good, thank you! I think we don't need to import fetch from undici, but probably it's a good idea to make sure the fetch and Agent implementation is on par.

I would create myFetch outside of the request call, and then set fetch: myFetch. That will make it more readable. Other than that I think that's great for a starting point, let's see what people say that try to use it.

@wolfy1339
Copy link
Member

I think we don't need to import fetch from undici, but probably it's a good idea to make sure the fetch and Agent implementation is on par.

The standard fetch types that typescript uses doesn't include the dispatcher option. We have to explicitly import fetch, and Agent in order for it to be proper types.

Agent is not even included in NodeJS, it's only part of undici. Only fetch is exposed as a global/
https://nodejs.org/api/globals.html#fetch

@github-actions
Copy link

🎉 This issue has been resolved in version 8.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Documentation Improvements or additions to documentation
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants