fix: use hosted valhalla default_style.json (#345)#388
fix: use hosted valhalla default_style.json (#345)#388Guptaji-007 wants to merge 3 commits intovalhalla:masterfrom
Conversation
|
Preview is ready! 🚀 You can view it here: https://valhalla-app-tests.gis-ops.com/388 |
nilsnolde
left a comment
nilsnolde
left a comment
There was a problem hiding this comment.
thanks, got a few questions/suggestions.
all in all it seems like too much extra code added, I'd have expected the diff to be smth like +58 -243 or so, not 226 lines added. but I didn't review the code bcs I suck at web dev, I'll let @mustaphaturhan judge that.
| export const VALHALLA_EDGES_LAYER_ID = 'valhalla-edges'; | ||
| export const VALHALLA_SHORTCUTS_LAYER_ID = 'valhalla-shortcuts'; | ||
| export const VALHALLA_NODES_LAYER_ID = 'valhalla-nodes'; | ||
| export const VALHALLA_DEFAULT_STYLE_URL = |
There was a problem hiding this comment.
let's make this a .env var so anyone running the app can easily change it to their own style
There was a problem hiding this comment.
sure, i will add this to the .env file.
There was a problem hiding this comment.
let the reviewers resolve conversations pls! otherwise it's really hard to check what has been done to resolve any change request!
There was a problem hiding this comment.
Got it, sorry about that — I’ll leave the conversations for reviewers to resolve going forward.
| vi.stubGlobal( | ||
| 'fetch', | ||
| vi.fn(async () => ({ | ||
| ok: true, | ||
| json: async () => ({ | ||
| layers: [ | ||
| { | ||
| id: 'edges', | ||
| type: 'line', | ||
| source: 'valhalla', | ||
| 'source-layer': 'edges', | ||
| }, | ||
| { | ||
| id: 'shortcuts', | ||
| type: 'line', | ||
| source: 'valhalla', | ||
| 'source-layer': 'shortcuts', | ||
| }, | ||
| { | ||
| id: 'nodes', | ||
| type: 'circle', | ||
| source: 'valhalla', | ||
| 'source-layer': 'nodes', | ||
| }, | ||
| ], | ||
| }), | ||
| })) | ||
| ); |
There was a problem hiding this comment.
hm, I have no idea about maplibre API, but this looks like style.json syntax. is this needed? I'd have imagine the URL is all it takes to configure the MVT source!
There was a problem hiding this comment.
The MVT source URL is enough to configure the vector tile source, but MapLibre still needs layer definitions (type, paint, layout, source-layer) to render the Valhalla overlays. In the current setup, the hosted default_style.json is being used as the source of those layer definitions. I’ve updated the test to mock the hosted-layer provider directly instead of stubbing fetch/raw style JSON inline.
|
hm, sorry, @mustaphaturhan just contacted from his vacation and noted that we shouldn't do this at all bcs of the complexity added. I don't understand it at this point, but we'll touch base next week or so about it. until then we'll need to pause here. sorry for that @Guptaji-007 ! |
I understand, thanks for the update! |
2 participants
🛠️ Fixes Issue
Closes #345
👨💻 Changes proposed
Updated Valhalla overlay layer loading to use the hosted upstream default_style.json
Removed locally maintained/manual overlay styling in favour of the hosted style definitions
Kept the implementation minimal, without adding fallback or extra feature work
Updated affected tests to match the hosted-style-based behavior
Hosted style source used:
https://raw.githubusercontent.com/valhalla/valhalla/master/docs/docs/api/tile/default_style.json
📄 Note to reviewers
I initially tested pointing the built-in MapLibre basemap style directly to the hosted file, but that JSON is a Valhalla overlay/debug style (edges, shortcuts, nodes) rather than a full basemap style. So this PR applies the hosted file in the Valhalla overlay styling path instead.
Validation done:
Ran targeted tests for the changed tile modules
All targeted tests passed locally
📷 Screenshots
Not included, since this change mainly updates the source of the Valhalla overlay style definitions rather than introducing a visible UI change.
AI Assitance:
Implemented with AI assistance, followed by manual review and local testing.