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

feat: Add Custom Fetcher Support to SimpleSDK Constructor #161

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

mindrunner
Copy link
Contributor

🚀 Description

This PR extends the SimpleSDK class constructor to support custom fetcher functions, allowing it to integrate seamlessly with a variety of HTTP libraries. Importantly, this change is backward-compatible, retaining support for existing axios and fetch configurations.

🤔 Why is this change important?

  • Versatility: Removes the limitation of only being able to use Axios or Fetch for HTTP requests.
  • Backward Compatibility: Old configurations with Axios and Fetch will continue to work as before.
  • User-Choice: Enables the developer to use other HTTP libraries like Superagent.

🛠 Technical Details

  • New Type Added: FetcherFunction defines the signature of the custom fetcher.
type FetcherFunction = <T>(params: any) => Promise<T>;
  • Extension in FetcherOptions Type: New fetcher option added.
type FetcherOptions = { axios: AxiosRequirement } | { fetch: typeof fetch } | { fetcher: FetcherFunction };
  • Fetcher Construction: New function constructFetcher() chooses the appropriate fetcher.
const constructFetcher = (options: FetcherOptions): FetcherFunction => {
    if ('axios' in options) return constructAxiosFetcher(options.axios);
    if ('fetch' in options) return constructFetchFetcher(options.fetch);
    return options.fetcher;
};

Example Implementation (superagent)

import { FetcherFunction } from '@paraswap/sdk/dist/types'
import superagent from 'superagent'

export const superagentFetcher = (): FetcherFunction =>
  async function <T>(params: any): Promise<T> {
    if (params.method.toLowerCase() === 'post') {
      const response = await superagent.agent.post(
        params.url,
        params.data,
        params.headers
      )

      return response.data
    } else {
      const response = await superagent.agent.get(params.url, params.headers)

      return response.data
    }
  }


const fetcher: FetcherFunction = superagentFetcher();
const sdk = constructSimpleSDK({ fetcher });

@Velenir
Copy link
Member

Velenir commented Sep 20, 2023

Hello @mindrunner !

Nice suggestion.
Can you create a src/examples/customFetcher.ts with a fully-realized example of using superagent 🙏
There please make use of FetcherError class for a reliable error structure and AbortSignal, also add a FetcherFunction reexport from src/index.

Btw, it is already possible to use a custom FetchFunction directly with constructPartialSDK, and for constructSimpleSDK you can make a typeof fetch function, something like this.

But having an extra option type FetcherOptions |= { fetcher: FetcherFunction }, like you suggest, would be best.

@mindrunner
Copy link
Contributor Author

Hi @Velenir,

Thank you for your prompt reply.

Can you create a src/examples/customFetcher.ts with a fully-realized example of using superagent?

Certainly, I can create that example. However, integrating superagent would make the SDK and its users dependent on that library. My initial objective was to potentially remove older functionalities, including dependencies on axios and fetch, in a future major release to streamline the SDK. If your vision differs, I'm open to implementing the superagent example.

Please share your thoughts. I'll proceed with the example in the coming days.

Best regards,
Lukas

@Velenir
Copy link
Member

Velenir commented Sep 22, 2023

integrating superagent would make the SDK and its users dependent on that library. My initial objective was to potentially remove older functionalities, including dependencies

You are right about that.
A customFetcher example doesn't need to depend specifically on superagent, and of course adding another devDependency isn't ideal. Still, having a more specific example that demonstrates use-case of a 3rd party library is best, I believe.
Your intent for custom fetcher is to be able to use it with a library, any library, capable of http requests, which I'm very much in favor of.
So how about we make an example with something generic but that would still reflect an actual library. Like this

Wdyt?

@mindrunner
Copy link
Contributor Author

Thanks for the example. I added it to the PR! :)

@Velenir
Copy link
Member

Velenir commented Oct 16, 2023

Great.
Just add a couple more changes and I'll be happy to merge this.

  1. Reexport FetcherError as a value (it is currently exported as a type only), so that it can be used in error handling in custom fetcher.
  2. Reexport FetcherFunction from src/index so it's accessible as a 1st level import to make creating customFetchers easier.

A good indication of whether you have reexported everything necessary is whether you can import all you need from /index:

//customFetcher.ts
import { constructSimpleSDK, FetcherError, FetcherFunction } from '../';

@mindrunner
Copy link
Contributor Author

mindrunner commented Oct 16, 2023

absolutely makes sense! Thanks for pointing out. Fixed it :)

@mindrunner
Copy link
Contributor Author

@Velenir Let me know if you need anything else to get this merged! :)

1 similar comment
@mindrunner
Copy link
Contributor Author

@Velenir Let me know if you need anything else to get this merged! :)

@Velenir
Copy link
Member

Velenir commented Oct 31, 2023

@Velenir Let me know if you need anything else to get this merged! :)

Thank you for pinging me.
Nothing else needed, I'll merge and make a new release today.

@Velenir Velenir merged commit e4ee7ac into paraswap:master Oct 31, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants