-
Notifications
You must be signed in to change notification settings - Fork 80
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
Update valhalla.js Graphhopper to V7 #948
base: dev
Are you sure you want to change the base?
Conversation
In API version 7 of graphhopper the support for vehicle parameter has been dropped. You have to use the profile parameter. see: https://github.com/graphhopper/graphhopper/blob/master/CHAN GELOG.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For users who are still using earlier versions of Graphhopper, should we support both parameters simultaneously?
Thanks for your work on this! Please let me know when you're ready for this to be reviewed. Don't worry about the e2e tests failing, it's because the PR originates from your repo |
You can review it now with the latest code. I change te code in the github online editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to do some testing on this. For now, please see cleanup comments
let profilecar = false | ||
let profileveh = true | ||
// Only check for definition not the value. | ||
if (process.env.GRAPH_HOPPER_V7) { | ||
profilecar = true | ||
profileveh = false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the value is always car
can we instead adjust params after it is set to either add a profile: car or a vehicle: car depending on the config?
} | ||
const locations = points.map(p => (`point=${p.lat},${p.lng}`)).join('&') | ||
// Avoiding motorways requires a POST request with a formatted body | ||
const avoidmotorwaysbody = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this configurable? Also please camel case
...(profilecar && { profile: 'car' }), | ||
...(profileveh && { vehicle: 'car' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-use the dynamic object key from earlier
Checklist
dev
before they can be merged tomaster
)Description
Version 7 of graphhopper api no longer supported vehicle, you have to use profile paramter instead.
See: https://github.com/graphhopper/graphhopper/blob/master/CHANGELOG.md