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

Update valhalla.js Graphhopper to V7 #948

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from

Conversation

mvanlaar
Copy link
Contributor

@mvanlaar mvanlaar commented May 3, 2023

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JSDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing
  • The description lists all relevant PRs included in this release (remove this if not merging to master)
  • e2e tests are all passing (remove this if not merging to master)

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

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
Copy link
Contributor

@miles-grant-ibigroup miles-grant-ibigroup left a 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?

lib/scenario-editor/utils/valhalla.js Outdated Show resolved Hide resolved
@miles-grant-ibigroup
Copy link
Contributor

miles-grant-ibigroup commented May 16, 2023

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

@mvanlaar
Copy link
Contributor Author

You can review it now with the latest code. I change te code in the github online editor.

Copy link
Contributor

@miles-grant-ibigroup miles-grant-ibigroup left a 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

Comment on lines +260 to +266
let profilecar = false
let profileveh = true
// Only check for definition not the value.
if (process.env.GRAPH_HOPPER_V7) {
profilecar = true
profileveh = false
}
Copy link
Contributor

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 = {
Copy link
Contributor

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

Comment on lines +287 to +288
...(profilecar && { profile: 'car' }),
...(profileveh && { vehicle: 'car' })
Copy link
Contributor

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

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.

2 participants