Skip to content

feat: Add ArcGIS Maps SDK support with custom basemap configuration#335

Open
ashum9 wants to merge 6 commits intoOneBusAway:developfrom
ashum9:develop
Open

feat: Add ArcGIS Maps SDK support with custom basemap configuration#335
ashum9 wants to merge 6 commits intoOneBusAway:developfrom
ashum9:develop

Conversation

@ashum9
Copy link

@ashum9 ashum9 commented Feb 8, 2026

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.

- 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
Copilot AI review requested due to automatic review settings February 8, 2026 10:41
@CLAassistant
Copy link

CLAassistant commented Feb 8, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ArcGISMapProvider implementing the map provider interface using @arcgis/core with optional custom vector tile basemap support.
  • Extends provider selection/configuration via MapSource, MapContainer, .env.example, and Vitest env mocks.
  • Adds the @arcgis/core dependency 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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Prettier formatting failures will block CI. Running npm run lint fails on src/components/MapContainer.svelte and src/lib/Provider/ArcGISMapProvider.svelte.js (trailing whitespace and long lines). Run npx prettier --write on both files to fix.

  2. Replace custom _decodePolyline() with existing polyline-encoded dependency. The project already depends on polyline-encoded, and the OSM provider uses PolylineUtil.decode() from it (see OpenStreetMapProvider.svelte.js:6). The 38-line hand-rolled decoder in ArcGISMapProvider.svelte.js:787-824 is duplicated code that could diverge from the library's behavior. Add import PolylineUtil from 'polyline-encoded'; at the top, replace this._decodePolyline(encodedShape) with PolylineUtil.decode(encodedShape) in createPolyline(), and delete the _decodePolyline method entirely.

  3. Remove redundant graphic.attributes assignment in addStopRouteMarker(). The attributes { stopId: stop.id, stopTime: stopTime } are set in the Graphic constructor (lines 358-361), then identically overwritten on lines 371-374. Delete lines 370-374 (the second assignment and its comment).

  4. Remove circular self-reference marker.htmlMarker = marker (line 592). This assigns a marker to itself, which is confusing and unnecessary. In _updateAllMarkerPositions(), the stopMarkers loop checking marker.htmlMarker is dead code (stop route markers use ArcGIS Graphics, not HTML overlays), and the vehicleMarkers loop can iterate directly without the .htmlMarker indirection. 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

  1. Add .catch() to hitTest() promise in addStopRouteMarker() (line 380). The shared click handler calls this.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 */ }).

  2. Initialize pinMarkers in the constructor. Every other array (stopMarkers, vehicleMarkers, polylines) is initialized in the constructor, but pinMarkers uses lazy initialization at line 504 (if (!this.pinMarkers) this.pinMarkers = [];) and a defensive fallback at line 275 (this.pinMarkers || []). Add this.pinMarkers = []; to the constructor and remove both guards.

  3. Handle view.when() rejection in initMap(). If await this.view.when() rejects (invalid API key, WebGL failure, bad basemap URL), this.view is left in a non-null but non-functional state. Every subsequent method passes the if (!this.view) return guard and tries to use the broken view, causing cascading failures. Wrap it in try/catch and null out this.view and this.map on failure before re-throwing:

    try {
        await this.view.when();
    } catch (error) {
        this.view = null;
        this.map = null;
        throw error;
    }
  4. Clean up vehicle popup watcher handles. In addVehicleMarker() (line 580), each click registers a new this.view.popup.watch('visible', ...) watcher, but previous handles are never removed. Over many clicks, these accumulate as memory leaks. Store the handle as this._vehiclePopupWatchHandle and call .remove() on the previous handle before registering a new one.

Fit and Finish

  1. Hide off-screen markers. In _updateMarkerPosition(), when view.toScreen() returns null (point is off-screen), the marker element keeps its last position. Add marker.element.style.display = 'none' when the point is off-screen, and marker.element.style.display = '' when it's visible. This prevents stale markers from appearing at wrong positions after panning.

  2. 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 returning null instead 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.

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.

3 participants