xapijs / xapi

Communicate over xAPI using JavaScript.
https://www.xapijs.dev/xapi-wrapper-library
MIT License
43 stars 21 forks source link

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

Closed scerelli closed 2 months ago

scerelli commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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.

attenzione commented 2 months ago

Any progress to release this feature?

CookieCookson commented 2 months ago

@attenzione The plan was to refactor this repository into a monorepo and release this change as part of that, paving way for xAPI 2.0. This was way back in April and since then I've lost a lot of my spare time!

In order to get this fetcher customisation functionality out as the refactor is a long way off, I will just do a major release (v3.0.0) and worry about the rest later on.

Keep your eyes peeled for a release 👀

CookieCookson commented 2 months ago

@scerelli @attenzione This has now been officially released in @xapi/xapi@3.0.0 🎉 . Please let me know how you get on with your implementations and if any further fixes need to be made!