feat: Add ArcGIS Maps SDK support with custom basemap configuration#335
feat: Add ArcGIS Maps SDK support with custom basemap configuration#335ashum9 wants to merge 6 commits intoOneBusAway:developfrom
Conversation
- Implement ArcGISMapProvider with full feature parity to existing providers - Support custom vector tile basemaps via PUBLIC_ARCGIS_CUSTOM_BASEMAP_URL - All 25+ provider interface methods implemented - Light/dark theme switching with ArcGIS built-in basemaps - Stop markers, vehicle markers, and trip planning pins - Polyline support with encoded shape decoding - Add @arcgis/core dependency with dynamic imports
There was a problem hiding this comment.
Pull request overview
Adds ArcGIS Maps SDK as an additional map provider option (alongside Google Maps and OpenStreetMap), including configuration via env vars and documentation updates.
Changes:
- Introduces
ArcGISMapProviderimplementing the map provider interface using@arcgis/corewith optional custom vector tile basemap support. - Extends provider selection/configuration via
MapSource,MapContainer,.env.example, and Vitest env mocks. - Adds the
@arcgis/coredependency and updates docs (README.md,CLAUDE.md) for ArcGIS setup.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/Provider/ArcGISMapProvider.svelte.js |
New ArcGIS-backed provider implementation for markers, vehicles, polylines, theme, bounds, and events. |
src/components/MapContainer.svelte |
Selects ArcGIS provider based on PUBLIC_OBA_MAP_PROVIDER and wires ArcGIS env vars. |
src/config/mapSource.js |
Adds ArcGIS: 'arcgis' to the provider enum. |
.env.example |
Documents new ArcGIS env vars (API key + optional custom basemap URL). |
vitest-setup.js |
Mocks new ArcGIS env vars for tests. |
package.json |
Adds @arcgis/core dependency. |
package-lock.json |
Lockfile updates from adding @arcgis/core dependency tree. |
README.md |
Adds ArcGIS configuration instructions. |
CLAUDE.md |
Updates provider architecture notes to include ArcGIS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ix event listeners
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… clarify arrow limitation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ts, clarify API key docs
aaronbrethorst
left a comment
There was a problem hiding this comment.
Hey Ashutosh, thanks for taking the time to add ArcGIS Maps SDK support to Wayfinder. This is a substantial contribution — implementing all 25+ provider interface methods with dynamic module loading for code splitting shows real attention to the project's architecture. The documentation updates to CLAUDE.md, README.md, and .env.example are thorough and appreciated. Before we can merge this, I will need you to make a couple changes:
Critical
-
Prettier formatting failures will block CI. Running
npm run lintfails onsrc/components/MapContainer.svelteandsrc/lib/Provider/ArcGISMapProvider.svelte.js(trailing whitespace and long lines). Runnpx prettier --writeon both files to fix. -
Replace custom
_decodePolyline()with existingpolyline-encodeddependency. The project already depends onpolyline-encoded, and the OSM provider usesPolylineUtil.decode()from it (seeOpenStreetMapProvider.svelte.js:6). The 38-line hand-rolled decoder inArcGISMapProvider.svelte.js:787-824is duplicated code that could diverge from the library's behavior. Addimport PolylineUtil from 'polyline-encoded';at the top, replacethis._decodePolyline(encodedShape)withPolylineUtil.decode(encodedShape)increatePolyline(), and delete the_decodePolylinemethod entirely. -
Remove redundant
graphic.attributesassignment inaddStopRouteMarker(). The attributes{ stopId: stop.id, stopTime: stopTime }are set in theGraphicconstructor (lines 358-361), then identically overwritten on lines 371-374. Delete lines 370-374 (the second assignment and its comment). -
Remove circular self-reference
marker.htmlMarker = marker(line 592). This assigns a marker to itself, which is confusing and unnecessary. In_updateAllMarkerPositions(), thestopMarkersloop checkingmarker.htmlMarkeris dead code (stop route markers use ArcGIS Graphics, not HTML overlays), and thevehicleMarkersloop can iterate directly without the.htmlMarkerindirection. Remove line 592 and simplify_updateAllMarkerPositions()to:_updateAllMarkerPositions() { for (const marker of this.markersMap.values()) { this._updateMarkerPosition(marker); } for (const marker of this.vehicleMarkers) { this._updateMarkerPosition(marker); } for (const marker of this.pinMarkers) { this._updateMarkerPosition(marker); } }
Important
-
Add
.catch()tohitTest()promise inaddStopRouteMarker()(line 380). The shared click handler callsthis.view.hitTest(event).then(...)without a.catch(). If the view is destroyed during the async operation, this produces an unhandled promise rejection. Add.catch((err) => { /* hitTest can fail if view is destroyed */ }). -
Initialize
pinMarkersin the constructor. Every other array (stopMarkers,vehicleMarkers,polylines) is initialized in the constructor, butpinMarkersuses lazy initialization at line 504 (if (!this.pinMarkers) this.pinMarkers = [];) and a defensive fallback at line 275 (this.pinMarkers || []). Addthis.pinMarkers = [];to the constructor and remove both guards. -
Handle
view.when()rejection ininitMap(). Ifawait this.view.when()rejects (invalid API key, WebGL failure, bad basemap URL),this.viewis left in a non-null but non-functional state. Every subsequent method passes theif (!this.view) returnguard and tries to use the broken view, causing cascading failures. Wrap it in try/catch and null outthis.viewandthis.mapon failure before re-throwing:try { await this.view.when(); } catch (error) { this.view = null; this.map = null; throw error; }
-
Clean up vehicle popup watcher handles. In
addVehicleMarker()(line 580), each click registers a newthis.view.popup.watch('visible', ...)watcher, but previous handles are never removed. Over many clicks, these accumulate as memory leaks. Store the handle asthis._vehiclePopupWatchHandleand call.remove()on the previous handle before registering a new one.
Fit and Finish
-
Hide off-screen markers. In
_updateMarkerPosition(), whenview.toScreen()returns null (point is off-screen), the marker element keeps its last position. Addmarker.element.style.display = 'none'when the point is off-screen, andmarker.element.style.display = ''when it's visible. This prevents stale markers from appearing at wrong positions after panning. -
getBoundingBox()fallback returns null-island coordinates. The{ north: 0, south: 0, east: 0, west: 0 }fallback (line 878) represents a point off the coast of Africa. Callers will use this to fetch stops at the wrong location. Consider returningnullinstead and having the caller handle it, or at minimum validate that the computed coordinates are finite before returning them.
Thanks again, and I look forward to merging this change.
Adds ArcGIS Maps SDK as a third map provider option alongside Google Maps and OpenStreetMap. This implementation enables organizations to use ArcGIS services and custom vector tile basemaps while maintaining full feature parity with existing providers.
Core Implementation
New Provider: ArcGISMapProvider.svelte.js (~840 lines)
Implements all 25+ provider interface methods
Dynamic module loading for optimal bundle size
Custom HTML marker rendering via overlay positioning
Support for encoded polyline decoding
Integration & Configuration
Added ArcGIS to MapSource enum in mapSource.js
Updated MapContainer.svelte with provider selection logic
Added environment variables to .env.example:
PUBLIC_ARCGIS_API_KEY - ArcGIS API key
PUBLIC_ARCGIS_CUSTOM_BASEMAP_URL - Optional custom basemap URL
Updated test mocks in vitest-setup.js
Added @arcgis/core dependency to package.json
Documentation
Updated README.md with ArcGIS configuration instructions
Updated CLAUDE.md with provider architecture details
Features
Stop markers with route labels and popups
Vehicle markers with real-time updates
Trip planning origin/destination pins
Polyline rendering with directional arrows
Light/dark theme switching (built-in basemaps)
Custom vector tile basemap support
Event handling (zoom, pan, bounds changes)
Usage
PUBLIC_OBA_MAP_PROVIDER="arcgis"
PUBLIC_ARCGIS_API_KEY="your-api-key"
PUBLIC_ARCGIS_CUSTOM_BASEMAP_URL="" # Optional
Testing Notes
No real API key required for basic testing (confirmed by maintainer feedback)
Dynamic imports ensure ArcGIS modules only load when provider is selected
Follows existing provider pattern for consistency
Stats
Note: Most additions (~2,040 lines) are from package-lock.json due to @arcgis/core dependency tree. Core implementation is ~840 lines in ArcGISMapProvider.svelte.js.