feat: add fetch override

[BartoszJarocki]

This PR renames fetch to fetchOptions and adds an additional option to provide own fetch method.

fixes #243

[lukeocodes]

Thanks, I really appreciate the effort here.

The only issue with this is that it is a breaking change and we're not ready to add another major version to our release channel. I would encourage you to add a new property (not breaking) like fetchClient.

I would then replace fetchWithAuth with a function that switched Fetch if it was supplied, and then called fetchWithAuth to return a preconfigured fetch instance using the right class and the auth headers.

Superbase has a good example, and they use cross-fetch too https://github.com/supabase/supabase-js/blob/master/src/lib/fetch.ts#L6

Also any tests you're able to provide would be very helpful

[davidvonthenen]

Clicked approve on the wrong PR. Should have been this one:
#246 (review)

[BartoszJarocki]

@lukeocodes understood, my thinking was that it'll be a bit confusing to have fetch as fetch options and something different to actually override fetch. now, I changed the implementation to be similar to supabase example, thank you! would you mind taking a look again?

regarding tests, I personally am not a fan of testing protected methods so the only option I see is to add tests like that to each client type

  it("should use custom fetch when provided", async () => {
    const fetch = async () => {
      return new Response(JSON.stringify({ customFetch: true }));
    };
 
    const client = createClient(faker.string.alphanumeric(40), {
      global: { url: "https://api.mock.deepgram.com" },
      customFetch: fetch,
    });
 
    const { result, error } = await client.manage.getProjectBalances(faker.string.uuid());
 
    assert.isNull(error);
    assert.isNotNull(result);
    assert.containsAllDeepKeys(result, ["customFetch"]);
  });

wdyt?

[lukeocodes]

Looks good to me! I'm going to personally test this on browsers, edge functions and Node environments to ensure we're not introducing a regression. I'll try and get this reviewed by the end of the week. Really appreciate you coming back with fixes for the PR

[BartoszJarocki]

@lukeocodes awesome, please let me know if there's anything missing!

[pkpio]

Any update on this? Would be great to also see an example somewhere of a custom fetch impl to use - trying to workaround the 5 mins timeout errors of the default nodejs fetch

[BartoszJarocki]

@pkpio I added some examples here https://github.com/BartoszJarocki/deepgram-js-sdk-custom-fetch/blob/main/index.ts

[myselfhimself]

@lukeocodes understood, my thinking was that it'll be a bit confusing to have fetch as fetch options and something different to actually override fetch. now, I changed the implementation to be similar to supabase example, thank you! would you mind taking a look again?

regarding tests, I personally am not a fan of testing protected methods so the only option I see is to add tests like that to each client type

  it("should use custom fetch when provided", async () => {
    const fetch = async () => {
      return new Response(JSON.stringify({ customFetch: true }));
    };
 
    const client = createClient(faker.string.alphanumeric(40), {
      global: { url: "https://api.mock.deepgram.com" },
      customFetch: fetch,
    });
 
    const { result, error } = await client.manage.getProjectBalances(faker.string.uuid());
 
    assert.isNull(error);
    assert.isNotNull(result);
    assert.containsAllDeepKeys(result, ["customFetch"]);
  });

wdyt?

Your test looks good, it could be added to https://github.com/deepgram/deepgram-js-sdk/blob/main/test/client.test.ts if I am not mistaken..

[lukeocodes]

I'll calve out time to review and test this next week!

[myselfhimself]

Hello here is some example working code using an AbortController .signal-enabled custom fetch with @BartoszJarocki 's pull request:

import { DeepgramError, createClient } from "@deepgram/sdk";
import { SettingsManagerCookies as SettingsManager } from "./settingsManager";

export class DeepgramClientCancellationError extends DeepgramError {
    originalError: unknown;

    constructor(message: string, originalError: unknown) {
        super(message);
        this.name = "DeepgramClientCancellationError";
        this.originalError = originalError;
    }
}

export const createDeepgramClient = (apiKey?: string, signal?: AbortSignal) => {
    async function cancellableFetch(
        input: RequestInfo | URL, init?: RequestInit
    ): Promise<Response> {
        // Use cross-fetch with the AbortController signal
        try {
            const response = await fetch(input, { ...init, signal });
            console.log("got response in custom Fetch, with signal" + JSON.stringify(signal));
            return response;
        } catch (error) {
            if ((error as DOMException).name === "AbortError") {
                // Handle fetch abort due to timeout
                throw new DeepgramClientCancellationError("Deepgram query aborted by user", error);
            } else {
                // Re-throw other errors
                throw error;
            }
        }
    }



    var proxyUrl = "http://127.0.0.1:8889";

    console.log("creating client with Deepgram experimental customFetch parameter");
    return createClient("proxy", {
        restProxy: { url: proxyUrl },
        fetch: { headers: { forceApiKey: apiKey ?? (SettingsManager.getApiKey()) } },
        customFetch: cancellableFetch
    });
}

// Example usage:
var controller = new AbortController();
var client = createDeepgramClient(undefined /* using my settings manager */, controller.signal);
client.listen.prerecorded.transcribeFile(binFile, options).then((result) => {}).catch((err) => { if(err.name == "AbortController") { console.log("was cancelled"); })
// Some cancelling somewhere, on some click() or interrupt.
controller.abort();

To use the custom fetch pull request, I used the following package.json line: "@deepgram/sdk": "github:deepgram/deepgram-js-sdk#pull/245", and I had to cd into node_modules/@deepgram/sdk and run npm i followed by npm run build.

The forceApikey mechanism allows a deepgram client to tell a deepgram proxy to use a different API key, it is described in the deepgram http proxy project., though no deepgram proxy implementation has been published yet IMHO.

[myselfhimself]

I am about to release a product with this fork...
Here are some information for people wanting to use the fork with package.json in a CI/CD pipeline:
package.json:

...
"scripts": {
"prebuild": "ls node_modules/@deepgram/sdk | grep -q tsconfig.json && npm i --prefix node_modules/@deepgram/sdk && npm run build --prefix node_modules/@deepgram/sdk
....}
...
"dependencies": {
...
    "@deepgram/sdk": "https://github.com/BartoszJarocki/deepgram-js-sdk/tarball/feat/add-fetch-override",
...
}
...

[lukeocodes]

@BartoszJarocki thanks for your patience on this. If you could please add the test just as you had it, that will be great.

FYI, I have modified the namespace of the custom fetch to _experimentalCustomFetch. I am keen not to take up any more namespaces before I figure out how I want to allow settings to differ between products on the same instance. We have some thinking to do (as we've added more products to the SDK).

Once we have the test, this is good to go. You can add it to client.test.ts. Thanks again for your patience and help.

[BartoszJarocki]

@lukeocodes I'm glad it's good to go! I just added a test 84a4e27.

[lukeocodes]

Sweet. I'll just double check some stuff, might cut a release today all being well and having a colleague available to check a box 😇 Thanks again for your help and patience! I appreciate it's been sitting here for a month or two, while we we cleared our plates!

…

On Thu, 25 Apr 2024, 18:13 Bartosz Jarocki, ***@***.***> wrote: @lukeocodes <https://github.com/lukeocodes> I'm glad it's good to go! I just added a test 84a4e27 <84a4e27> . — Reply to this email directly, view it on GitHub <#245 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHJPAWM7NC5MPSLWEZOFBLY7E2U5AVCNFSM6AAAAABCXGBSRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZXG43TMNRRGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[BartoszJarocki]

awesome, sounds good!

[myselfhimself]

Now using the 3.3.0 version, with a unit test to ensure the experimental custom fetch parameter keeps working before releasing myself. Thank you @BartoszJarocki @lukeocodes !!

[lukeocodes]

Thanks again!

I have now (now that I have some time) been looking at a long term solution for transmitter overrides (websocket and fetch), custom headers, namespaced config per product (transcription and speech intelligence can operate differently), etc, etc.

I have a good solution that is backwards compatible, and will maintain compatibility with the experimental property we included here.

Mind if I include you in the review of it when it is ready?

[myselfhimself]

Good!