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

Use axios consequently for all HTTP requests in Wegue #400

Closed
chrismayer opened this issue Jun 11, 2024 · 5 comments · Fixed by #416
Closed

Use axios consequently for all HTTP requests in Wegue #400

chrismayer opened this issue Jun 11, 2024 · 5 comments · Fixed by #416

Comments

@chrismayer
Copy link
Collaborator

Since axios is added to Wegue as dependency we should consequently use axios to perform HTTP requests. So usage of other techniques like fetch, XMLHttpRequest, ... should be replaced by axios calls.

Herewith axios should be upgraded to its latest version.

This issue is based on this discussion.

@sronveaux
Copy link
Collaborator

Hi @chrismayer, hi @fschmenger,

I was looking through upgrading the Geocoder component when I thought it was clearly linked with this one.
It seems Geocoder is the last place where HTTP requests are not made through Axios...

Before working on it, I wanted to discuss here with both of you what should stay and could be removed... this component is "relatively" complicated because it either uses good old JSON-P or fetch depending on the configuration returned by the provider...
Currently, none of the stock providers are making use of JSON-P anymore and this technique, as you certainlky know, even though it was necessary and very useful at a time is now more than deprecated, mainly because of its security concerns.

My real question was: do you really think we should keep the JSON-P possibility or could it be completely removed ? If you want to keep it, do you really know a safe geocoding server which still uses this technology to make some tests ?

Thanks in advance to give your opinion here to aim to the best solution with this issue.

Cheers

@fschmenger
Copy link
Collaborator

Hi @sronveaux,
I never worked on geocoder, so i don't have any insights here. Maybe @justb4 could help you out.
Regarding JSON-P it was already considered deprecated a couple of years ago. If you can`t find any providers then it is maybe no longer supported? Is there anything in the docs of the respective services?

Anyway very much appreciated if you work on it.
Cheers Felix

@sronveaux
Copy link
Collaborator

Hi @fschmenger,
Thanks for your valuable reply.

In fact, in the three services available be default in Wegue (OpenCage, Photon and Nominatim), only Nominatim still mentions it in that you can pass a json_callback parameter in order to force a JSON-P answer back... but everything works flawlessly without it though!

From my point of view, this JSON-P things should be removed for security reasons at first, but I don't want to break something or remove a feature if some people have developed custom geocoder providers which necessitates JSON-P. That's why I asked before going further...

@chrismayer
Copy link
Collaborator Author

Hi @sronveaux, I am in favor to remove JSONP support for the already mentioned reasons by you and @fschmenger. It had its time but now it has to go. We should mention this in the upgrade-notes.md so it gets transparent.

Thank for picking this up 👍

@sronveaux
Copy link
Collaborator

Thanks @chrismayer, you wrote exactly what I hoped to read 😄

As summer break is coming, I will certainly not begin this before end of August or September... and if someone else want to tackle it inbetween, we're set on the path to take...

Cheers

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