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

There should be a way to customise the fetcher. #382

Open
scerelli opened this issue Mar 25, 2024 · 10 comments · May be fixed by #383
Open

There should be a way to customise the fetcher. #382

scerelli opened this issue Mar 25, 2024 · 10 comments · May be fixed by #383

Comments

@scerelli
Copy link

I believe there is a significant limitation due to the lack of an option to customize the fetcher. In today's environment, where services are deployed across various platforms, the inability to use a custom fetcher is restrictive. For instance, my service, which utilizes where i wrote a wrapper of xapi.js needs to operate both at the edge and in the browser.

While Axios performs adequately for handling fetch requests in the browser, it does not cater as well to server-side applications, particularly in contexts like edge computing environments.

This limitation is a major blocker for integrating Axios into diverse operational contexts, as detailed in issue #acops/axios/5523.

Could we discuss the possibility of introducing a feature that allows for custom fetcher implementations? or maybe better, drop axios in favor of the always working fetch.

What do you think?

@CookieCookson
Copy link
Collaborator

Hi @scerelli, thank you for your suggestions on how xAPI.js could be improved for operating in an edge environment.

I must admit I haven't tried operating the library in this type of environment, originally Axios was chosen as the adapter of choice because it was able to operate in both client and server based environments.

I would love to expand on the capabilities of the library without negatively impacting the existing user base, in particular where some users are hooking into the internal Axios instance, intercepting etc.

Thinking out loud, having the choice of adapter from some presets or the option to provide a custom adapter feels like a good option to me, which would allow the unbundling of axios as a direct dependency.

@scerelli
Copy link
Author

scerelli commented Mar 26, 2024

@CookieCookson thanks to you.

Please review the issue in detail, as customizing the adapters does not appear to resolve the problem. I have also attempted to explicitly use http instead of xhr directly within the library, but this approach was unsuccessful because axios presumes that in a node environment you have http module available while on the edge is not always true, most of the case is not true, so it won't never work.

A potential solution could be allow the passing down of a custom fetcher. This way, users can continue utilizing the library as usual, while also having the option to customize for more specific use cases.

Additionally, there is a good modern alternative to Axios: xior or get-it. Xior mimics Axios's implementation but is significantly lighter and solely uses the fetch API.

You can also have a look at this library sanity-io. It is quite good and it works all environments, node, browser and on the edge.

@CookieCookson
Copy link
Collaborator

I understand, you need the option to choose the adapter/fetcher or even provide a custom one. I'll get to work experimenting with being able to provide a config option to pass in a custom request handler method or choose from some pre-built internal options (e.g. fetch or axios).

@CookieCookson CookieCookson linked a pull request Mar 28, 2024 that will close this issue
12 tasks
@CookieCookson
Copy link
Collaborator

@scerelli Would you be up for giving an initial alpha version a test for me within an edge environment? I've not been successful in getting an edge test runner in place for jest so far. I've published a potential candidate to NPM as @xapi/xapi@2.3.0-alpha.0. You can now configure the adapter property in the XAPI constructor params:

const xapi = new XAPI({
adapter: 'fetch', // Default is `axios`
});

@CookieCookson
Copy link
Collaborator

I've spun up a local cloudflare wrangler project and utilised the production version of @xapi/xapi and managed to reproduce the issue you're facing. From using the alpha version above and switching the adapter to fetch, I was able to successfully complete a getAbout request and return it.

It would be great if you did have the time to see if the alpha version above works for your use case. I'll continue to wrap up the feature and have another go at seeing if I can simulate the edge worker environment in jest.

@scerelli
Copy link
Author

scerelli commented Apr 19, 2024

@scerelli Would you be up for giving an initial alpha version a test for me within an edge environment? I've not been successful in getting an edge test runner in place for jest so far. I've published a potential candidate to NPM as @xapi/xapi@2.3.0-alpha.0. You can now configure the adapter property in the XAPI constructor params:

const xapi = new XAPI({
adapter: 'fetch', // Default is `axios`
});

sorry i have missed your message, I will try with my aws/vercel env and I let you know

@scerelli
Copy link
Author

@scerelli Would you be up for giving an initial alpha version a test for me within an edge environment? I've not been successful in getting an edge test runner in place for jest so far. I've published a potential candidate to NPM as @xapi/xapi@2.3.0-alpha.0. You can now configure the adapter property in the XAPI constructor params:

const xapi = new XAPI({
adapter: 'fetch', // Default is `axios`
});

Thanks for the great PR. It works quite well in my environment, i think it's a good starting point

@CookieCookson
Copy link
Collaborator

@scerelli Thanks for taking the time to test it out, and I am glad it works well in your environment. You mention it's a good starting point, is there anything else to consider which doesn't work as anticipated in the edge environment? If you need functionality to provide your own custom fetcher, the adapter property also supports a function with the AdapterFunction interface, where you can pass in your own method instead based off the fetch or axios adapter examples:

https://github.com/xapijs/xapi/blob/feature/adapters/src/adapters/fetchAdapter.ts
https://github.com/xapijs/xapi/blob/feature/adapters/src/adapters/axiosAdapter.ts

@scerelli
Copy link
Author

@CookieCookson I said it's a good starting point because I missed the AdapterFunction from the PR, I only quick checked fetch and tried it.

Great stuff!

@CookieCookson
Copy link
Collaborator

@scerelli Thats great to hear!

The work for adapters is complete now, I am just figuring out how much it impacts existing users of the getAxios method which has been removed in this feature. I will most likely need to release this as part of a new major version as that is a breaking change.

I am also considering removing Axios from being the default adapter in favor of fetch and also unbundle it from this package to reduce the bundle size and dependencies, but that could have a much larger (and possibly negative) impact on existing users.

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 a pull request may close this issue.

2 participants