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

Replace axios with undici #283

Open
1 of 4 tasks
benmccann opened this issue Oct 11, 2024 · 8 comments
Open
1 of 4 tasks

Replace axios with undici #283

benmccann opened this issue Oct 11, 2024 · 8 comments
Labels

Comments

@benmccann
Copy link

Is your feature request related to a problem?

axios is a somewhat weighty dependency and doesn't adhere to the fetch standard

https://npmgraph.js.org/?q=axios

Describe the solution you'd like

Switch to undici. undici is the official fetch implementation bundled with Node.js. This should be an easy change as fetch is already used where available: https://github.com/PostHog/posthog-js-lite/blob/4fe4eb2aa5da5ec57f39481e57a712c6992991b4/posthog-node/src/fetch.ts#L17C10-L17C15

It also would make an eventual migration to Node's built-in fetch easy as you could simply drop the import and use the one from Node

Describe alternatives you've considered

It would be nice to use fetch directly from Node. However, doing so requires Node 18 and this library still supports Node 15. Could we drop support for EOL versions of Node?

Related sub-libraries

  • All of them
  • posthog-web
  • posthog-node
  • posthog-react-native

Additional context

Thank you for your feature request – we love each and every one!

@benmccann benmccann added the enhancement New feature or request label Oct 11, 2024
@marandaneto
Copy link
Member

@benmccann thanks for the suggestion.
You can replace the fetch function to not use the global fetch nor axios by setting this function if you wish for now.

@Lehoczky
Copy link

Lehoczky commented Nov 24, 2024

@benmccann thanks for the suggestion. You can replace the fetch function to not use the global fetch nor axios by setting this function if you wish for now.

Thanks for the reply @marandaneto! Unfortunately axios is still downloaded into node_modules with this approach, and makes this package quite a bit bigger than it could be.

If you look at npmgraph.js.org/?q=posthog-node, you can see posthog-node is 880KB in itself, but with its dependencies it is 3MB, and 2MB from that is axios. Removing it would reduce the time it takes to install the package, which can add up when someone is using CI workflows that can run several times a day.

Given that node v15 end of life was reached 3,5 years ago, and the current LTS is v22, it might be worth raising the minimum supported node version above v18, and just remove axios, since fetch is natively available from v18.

@marandaneto
Copy link
Member

@Lehoczky is there any stats about the % of people/apps using node < 18 and node >= 18, although I agree with the approach and I think its a good idea, its important to keep compatibility in mind, not every person/app migrates that fast.

@marandaneto
Copy link
Member

you can probably use yarn resolutions to not download axios dependency if you are using the approach mentioned here.
npm has a similar approach as well with overrides

@benmccann
Copy link
Author

Node 18 active support ended on 18 Oct 2023 and Node 18 security support ends on 30 Apr 2025. Maybe we should just wait until May to do this?

@kravetsone
Copy link

I agree with this issue

We should migrate from axios. Ideally to cross-runtime built-in fetch

But for now i guess we can import it from undici (anyway nodejs fetch is undici fetch)

axios is big in size and bugly sometimes

@kravetsone
Copy link

I agree with this issue

We should migrate from axios. Ideally to cross-runtime built-in fetch

But for now i guess we can import it from undici (anyway nodejs fetch is undici fetch)

axios is big in size and bugly sometimes

image

fetch added in v16.15.0 (flagged) so i guess we can swtich away from axios right now. But it would be a major release

@kravetsone
Copy link

@Lehoczky is there any stats about the % of people/apps using node < 18 and node >= 18, although I agree with the approach and I think its a good idea, its important to keep compatibility in mind, not every person/app migrates that fast.

https://nodedownloads.nodeland.dev/

looks like nodejs <18 not so widely used for now

also fetch can be used on v16.15.0+ if u really wants

and NodeJS <18 is out from maintance status

i guess we should drop axios soon. it really important to achieve really tiny package

its my dream...

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

No branches or pull requests

4 participants