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

💡 [REQUEST] - Restore ability to inject custom "fetch" instance #1124

Closed
ifavo opened this issue Aug 1, 2024 · 2 comments · Fixed by #1535
Closed

💡 [REQUEST] - Restore ability to inject custom "fetch" instance #1124

ifavo opened this issue Aug 1, 2024 · 2 comments · Fixed by #1535

Comments

@ifavo
Copy link
Contributor

ifavo commented Aug 1, 2024

Summary

In the previous SDK releases it was possible to provide a custom axios instance to HttpClient.

This made it possible to use existing modules like https://axios-cache-interceptor.js.org/, to add caching functionality for immutable http requests to nodes.

I used it to cache immutable http requests in redis and return data for quicker responses within an RPC proxy.

With beta 23 axios was replaced with fetch, which is a good alternative, but as regression it is now no longer possible to just inject the handler (in this case fetch) as dependency to achieve the same result.

I propose to add the same functionality with fetch, so its possible to add a custom fetch handler. It would also allow to send custom headers for authentification or other actions to node backends, if neccessary.

As a workaround it might be possible to provide a custom HttpClient but that would be an unnecessary overhead.

Basic Example

import fetch from 'node-fetch-cache';

const httpClient = new HttpClient(options.node, { fetch })
const thorClient = new ThorClient(httpClient)
const provider = new VeChainProvider(thorClient);
@victhorbi
Copy link
Collaborator

Hey @ifavo , this issue will be worked on after the first official release of the SDK.
We will do it once the VeChain Data Model will be implemented to avoid having to adapt it after.

@victhorbi victhorbi added this to the Audit milestone Sep 30, 2024
@victhorbi victhorbi changed the title 💡 [REQUEST] - Restore ability to inject custom "fetch" instance 💡 [REQUEST] - Restore ability to inject custom "axios" instance Nov 14, 2024
@victhorbi victhorbi modified the milestones: 1.x (Audit), 1.1.0 Nov 14, 2024
@victhorbi victhorbi changed the title 💡 [REQUEST] - Restore ability to inject custom "axios" instance 💡 [REQUEST] - Restore ability to inject custom "fetch" instance Nov 14, 2024
@lucanicoladebiasi lucanicoladebiasi linked a pull request Nov 25, 2024 that will close this issue
14 tasks
@lucanicoladebiasi
Copy link
Collaborator

The VeChain SDK doesn't use node-fetch-cache nor node-fetch but the simpler Fetch API.

Albeit node-fetch-cache is based on node-fetch and the latter is based on the Fetch API, node-fetch is a library designed for NodeJS runtime hence it could be introduce runtime incompatibility when the SDK is used to develop software for other JS runtimes.

The Fetch API is tagged Baseline Widely available by MDN.

I am skeptic about the convenience to leave the availability of Fetch API.

PR at #1535 introduces the possibility to set HTTP request headers for the SimpleHttpClient objects using the Fetch API internally. Headers allow to manage cookies and to control the cache policies. Hoping this functionality is sufficient, this should be a convenient way to preserve the simplicity of the code, the compatibility with the existing code and the wider JS runtime panorama.

If the solution is not sufficient, we should provide a module offering an HttpClient implementation based on node-fetch-cache.

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

Successfully merging a pull request may close this issue.

3 participants