wegue-oss / wegue

Template and components for webmapping applications with OpenLayers and Vue.js
BSD 2-Clause "Simplified" License
93 stars 41 forks source link

Remove axios dependency? #394

Closed chrismayer closed 4 weeks ago

chrismayer commented 1 month ago

Currently the axios dependency is included into Wegue. It is used in the following files/modules:

IMHO using axios is a rather big buy-in to do some HTTP GET-requests. This could be easily done by the built-in fetch-API. By using fetch we could avoid maintaining another dependency, by this reduce vulnerabilities and decrease build size.

Any thoughts on this? Do I oversee something? Thanks for any input.

fschmenger commented 1 month ago

Hi Christian, it's been a long time ago since we moved towards axios so I don't remember the details: Here`s the PR that introduced most of it #279. It was originally proposed by Jakob within this PR #269.

I do not mind using axios. But it would be good to use either fetch or axios throughout the whole project. This would ensure consistency.

I cannot recall whether fetch had some restrictions at that point, e.g. poor support of cross origin requests. I think in the older days we even had to use JSONP.

Do we consider axios as a problem, how big is the package as of 2024? To quote myself from #269:

Are we good to include the axios library in the package? It's fairly lightweight (like 20kB extra) and it will ease our life in the future.

Hope this gives you a starting point for further considerations.

fschmenger commented 1 month ago

After scratching my head and reading through this comparison https://blog.logrocket.com/axios-vs-fetch-best-http-requests/ :

Whether these points stand by 2024 I cannot tell. We would have to re-implement stuff and test it. Cheers Felix

chrismayer commented 1 month ago

Hi @fschmenger , thanks for the detailed information and sharing your thoughts :+1:

Still not sure, if we really "need" the axios library, but maybe it is not worth the work to re-implement everything with fetch. Especially if it is really that tiny. According to bundlephobia it is 13.2 kB gzipped. I guess browser compatibility is no more the case but having some convenience layer - e.g. for request cancellation - is surely nice at some point.

But then we should use axios in all spots, where we perform HTTP requests (as you also stated). And upgrade to its latest version.

sronveaux commented 1 month ago

Hi team,

Just wanted to give my point of view here even though I don't have the history of its inclusion in Wegue.

For sure, as @chrismayer said, Axios is not needed in itself and could be removed, using fetch API instead.

However, I'd personally stick with it (and would by the way almost systematically add it in my custom apps) for many reasons. To name just a few of those reasons, due to the many features it has to offer if you need to make some API calls inside one app, due to built-in security protections (again if you need to make API calls) and due to the clarity of the written code and it's better error handling compared to fetch.
Yes, fetch is simpler but you have so much tests to do even if the result is a "success" to ensure it really is and to get the requested data back.

However, I totally agree that if keeping it is the chosen path, it should be used everywhere in a consistent way.

chrismayer commented 4 weeks ago

Thanks for your feedback @sronveaux! One good point you mentioned is the fact that axios could be intensively around in custom Wegue app code and removing the dependency could cause expense and annoyance for app devs. So I think we should stay with axios.

I created an issue to ensure consequent usage of axios in the Wegue code base :arrow_right: #400

So going to close this. Thanks to all of you for your feedback and thoughts :+1: