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

Avoid using jsonp for GraphHopper #1872

Closed
wants to merge 2 commits into from

Conversation

karussell
Copy link
Contributor

jsonp should be avoided as error reporting is not always possible. The necessary magic should be done from jquery automatically. (is there a possibility to test this on an openstreetmap.org domain before going production?)

@tomhughes
Copy link
Member

Well as far as I know we were only using jsonp because XHR wasn't possible - we certainly prefer not to use it. Is there a suitable cross domain policy in place that will allow access?

Also you will need to update https://github.com/openstreetmap/openstreetmap-website/blob/master/app/controllers/application_controller.rb#L411 to move graphopper from script_src to connect_src.

@karussell
Copy link
Contributor Author

Is there a suitable cross domain policy in place that will allow access?

Yes, it was reported it works for others :)

For https://graphhopper.com/api/1/info?key=_KEY_ you can see:

access-control-allow-origin=*
access-control-expose-headers=X-RateLimit-Limit,X-RateLimit-Remaining,X-RateLimit-Reset,X-RateLimit-Credits

And
curl -v -XOPTIONS 'https://graphhopper.com/api/1/info?key=_KEY_'
reports some more Access-Control headers

to move graphopper from script_src to connect_src

done

@tomhughes tomhughes closed this in 1cea6b3 May 17, 2018
@karussell
Copy link
Contributor Author

Thanks!

@karussell karussell deleted the patch-5 branch May 17, 2018 21:05
@karussell
Copy link
Contributor Author

When will it be deployed so that I can test if it still works?

@tomhughes
Copy link
Member

It's been tested, and has been deployed - it should be live by now.

@karussell
Copy link
Contributor Author

Wow, that was fast. Thanks a bunch - also for removing the other jsonp parameter! Works 👍

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 this pull request may close these issues.

None yet

2 participants