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

Allow custom fetch function to be provided #1646

Merged
merged 6 commits into from
Jun 27, 2024

Conversation

iammminzzy
Copy link
Contributor

@iammminzzy iammminzzy commented Jun 7, 2024

What does this PR do?

This PR allows a custom fetch function to be provided. This makes it a lot easier to add proxy support without having to copy the implementation (and remember to keep it up-to-date) of IsomorphicFetchHttpLibrary along with all of its dependencies:

import {
HttpLibrary,
RequestContext,
ResponseContext,
ZstdCompressorCallback,
} from "./http";
import { fetch as crossFetch } from "cross-fetch";
import pako from "pako";
import bufferFrom from "buffer-from";
import { isBrowser, isNode } from "../util";
import { logger } from "../../../logger";

Example usage:

import { ProxyAgent, fetch } from 'undici';

const configuration = client.createConfiguration({
  fetch: (input, init) =>
    fetch(input, { ...init, dispatcher: new ProxyAgent('...') })
});

Additional Notes

Review checklist

Please check relevant items below:

  • This PR includes all newly recorded cassettes for any modified tests.

  • This PR does not rely on API client schema changes.

    • The CI should be fully passing.
  • Or, this PR relies on API schema changes and this is a Draft PR to include tests for that new functionality.

    • Note: CI shouldn't be run on this Draft PR, as its expected to fail without the corresponding schema changes.

@iammminzzy iammminzzy requested a review from a team as a code owner June 7, 2024 14:53
/**
* Custom `fetch` function
*/
fetch?: typeof crossFetch;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether this is the right type or I should just use any (for example https://github.com/octokit/request.js allows you to pass a custom fetch function but types it as any: https://github.com/octokit/types.ts/blob/b21cc042affd15918abdc0636cb017e3751bbadc/src/Fetch.ts) because the Node fetch and browser fetch types aren't fully compatible: nodejs/undici#1943

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, any sounds a little safer to me.

Choose a reason for hiding this comment

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

Agreed! Feel free to push edits to this PR directly if you have edit rights

@NMinhNguyen
Copy link

@skarimo, @HantingZhang2, apologies for the direct ping but could you review this PR when you get a chance please?

@NMinhNguyen
Copy link

cc @therve

@skarimo
Copy link
Member

skarimo commented Jun 21, 2024

@NMinhNguyen 👋 We will review this soon and follow up.

@jack-edmonds-dd jack-edmonds-dd merged commit c09810e into DataDog:master Jun 27, 2024
13 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 27, 2024
* Allow custom `fetch` function to be provided

* Switch type to any.

* Fix test.

* Fix test.

---------

Co-authored-by: Hanting ZHANG <42933443+HantingZhang2@users.noreply.github.com>
Co-authored-by: Jack Edmonds <jack.edmonds@datadoghq.com> c09810e
@NMinhNguyen
Copy link

@jack-edmonds-dd thanks a lot for the updates and merging the PR! Roughly when can we expect a new release to be published?

@jack-edmonds-dd
Copy link
Contributor

@jack-edmonds-dd thanks a lot for the updates and merging the PR! Roughly when can we expect a new release to be published?

Probably early next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants