-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add basemap-browser example application #9892
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
base: master
Are you sure you want to change the base?
Conversation
- Comprehensive test application for deck.gl with multiple basemap providers - Supports Google Maps, Mapbox, MapLibre, and MapLibre Globe - Both Pure JS and React implementations for complete coverage - Interleaved mode toggle for testing shared vs separate GL contexts - Real-time metrics panel showing DPR, canvas dimensions, and drawing buffer - Complete isolation between Pure JS and React implementations - Fixed globe projection in interleaved mode (set projection before overlay) - Labels render above deck.gl layers in interleaved mode using beforeId/slot - TypeScript throughout with full type safety - Covers all 16 test scenarios from resize/DPR bug fix
- Pure JS code now mounts directly to DOM with no React wrapper - Control panel is separate React root that calls callback function - Map area is either Pure JS mount OR separate React root - Complete isolation between Pure JS and React implementations - Removed app.tsx, map-container.tsx, pure-js-container.tsx - Created control-panel.tsx standalone component - Created examples-react/ directory with separate components: - google-maps-component.tsx - mapbox-component.tsx - maplibre-component.tsx - Updated index.html with side-by-side layout (#controls and #map divs) - index.tsx orchestrates two separate React roots - No more React wrapping Pure JS code - true isolation achieved
- Defer cleanup and remounting to next tick using setTimeout - Prevents synchronous unmount during React render phase - Fixes 'Attempted to synchronously unmount a root' warning - Fixes 'Failed to execute removeChild' error - Extracted mountExample function for cleaner separation
felixpalmer
left a comment
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.
Nice! Given that we have a screenshot widget now, would it be possible to somehow run through all the variants and display a set of thumbnails as a sort of smoke test? Alternatively, could we somehow include these in our automated tests?
I think this is sort of possible! We could add the line of code from the screenshot widget that captures the canvas, and collect screenshots. Similar to a render test it'd go one by one through the examples to collect the results. This would really only be applicable to interleaved renders since those can capture both basemap and deck. Additionally, we could add a maplibre-based render test for the globe and Mercator variants (the others would require API keys to run the tests, which doesn't sound great) |
|
I gave the thumbnails idea a try and ran into issues where the images were empty unless I was actively interacting with the maps. I think something is clearing the buffers, but I'm not sure what since I've tried all of the usual suspects. I'm going to merge this in for now - happy to share what I tried if you're curious and want to give it a shot |
| overlay.setMap(map); | ||
| } | ||
| return () => overlay.setMap(null); | ||
| }, [map, overlay]); |
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.
Google Maps React overlay missing finalize() causes resource leak
Medium Severity
The GoogleMapsDeckOverlay component creates a new GoogleMapsOverlay via useMemo when interleaved changes, but the cleanup only calls overlay.setMap(null). Unlike the Pure JS implementation which calls overlay.finalize(), this React component never finalizes the overlay. When toggling the interleaved mode or unmounting, WebGL resources and internal state held by the old overlay are not properly released, potentially causing resource leaks. The finalize() method is specifically designed to release these resources.
| const MAPBOX_TOKEN = process.env.MapboxAccessToken; | ||
|
|
||
| // Mapbox Overlay wrapper | ||
| function MapboxOverlayWrapper(props: MapboxOverlayProps & {interleaved: boolean}) { |
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.
| function MapboxOverlayWrapper(props: MapboxOverlayProps & {interleaved: boolean}) { | |
| function MapboxDeckOverlay(props: MapboxOverlayProps & {interleaved: boolean}) { |
| @@ -0,0 +1,26 @@ | |||
| { | |||
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.
See if we can extend the existing config?
| import 'maplibre-gl/dist/maplibre-gl.css'; | ||
|
|
||
| // MapLibre Overlay wrapper | ||
| function MapLibreOverlay(props: MapboxOverlayProps & {interleaved: boolean}) { |
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.
MapLibreDeckOverlay
| }); | ||
|
|
||
| return () => { | ||
| deckOverlay.finalize(); |
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.
Necessary, or will map.remove do this already?
| map.addControl(new mapboxgl.NavigationControl()); | ||
|
|
||
| return () => { | ||
| deckOverlay.finalize(); |
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.
Ditto
| - Resize and DPR changes work in both modes | ||
| - Layers render correctly in both modes | ||
|
|
||
| ## Test Matrix |
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.
Format as table
examples/basemap-browser/README.md
Outdated
| ### Key Design Decisions | ||
|
|
||
| 1. **TypeScript Throughout**: All files use TypeScript for type safety | ||
| 2. **React Function Components**: No class components, uses hooks for state management | ||
| 3. **Shared Layer Configs**: Layer definitions extracted from get-started examples into `layers.ts` | ||
| 4. **Type-Safe Examples**: Example configurations are fully typed via `types.ts` |
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.
Remove
| ## Architecture | ||
|
|
||
| The basemap-browser uses TypeScript and React function components with a modular architecture: | ||
|
|
||
| ``` | ||
| src/ | ||
| ├── types.ts # TypeScript type definitions | ||
| ├── constants.ts # Shared constants from get-started examples | ||
| ├── layers.ts # Layer configurations (from get-started examples) | ||
| ├── examples/ | ||
| │ └── index.ts # Example configurations matching get-started | ||
| ├── app.tsx # Main app (React function component) | ||
| ├── map-container.tsx # Map rendering (React function components) | ||
| └── index.tsx # Entry point | ||
| ``` |
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.
Out of date?
|
@copilot I left a series of comments on this PR I'd like you to address |
|
@chrisgervang I've opened a new pull request, #9934, to work on those changes. Once the pull request is ready, I'll request review from you. |
| ? mapType === 'mapbox' | ||
| ? {slot: 'middle'} | ||
| : {beforeId: 'watername_ocean'} | ||
| : {}; |
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.
Incorrect interleaved props applied to Google Maps layers
Low Severity
The interleavedProps logic only checks for mapbox, falling through to {beforeId: 'watername_ocean'} for all other map types including 'google-maps'. The comment correctly states "Mapbox uses slot, MapLibre uses beforeId" but doesn't mention Google Maps because Google Maps handles interleaved mode differently and doesn't use beforeId at all. When Google Maps examples call getAirportLayers(interleaved, 'google-maps') with interleaved=true, they receive the MapLibre-specific beforeId prop which is meaningless for that platform.
Additional Locations (1)
|
|
||
| return () => { | ||
| map.remove(); | ||
| }; |
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.
Missing cancellation flag in MapLibre async callback
Low Severity
The map.on('load') callback lacks a cancellation check that the PR reviewer explicitly requested ("Add a cancellation flag"). Unlike google-maps.ts which uses a cancelled flag to prevent async callbacks from running after cleanup, maplibre.ts has no such protection. If the cleanup function is called before the map finishes loading, the on('load') callback could still fire and attempt to call addControl on a removed map, potentially causing errors.
|
|
||
| export default function GoogleMapsComponent({example, interleaved}: GoogleMapsComponentProps) { | ||
| // eslint-disable-next-line no-process-env | ||
| const apiKey = process.env.GoogleMapsAPIKey!; |
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.
React components missing API key validation unlike pure-js counterparts
Low Severity
The React components for Google Maps and Mapbox don't validate whether API keys are configured, while their pure-js counterparts show user-friendly error messages when keys are missing. GoogleMapsComponent uses a non-null assertion (!) on GoogleMapsAPIKey and MapboxComponent passes potentially undefined MapboxAccessToken directly to the map. When these environment variables aren't set, users will see cryptic runtime errors from the map libraries instead of the helpful "Configuration Required" messages that the pure-js implementations display.
Summary
This PR adds a comprehensive basemap-browser example application for testing deck.gl with multiple basemap providers across different frameworks and rendering modes.
Features
Use Case
This application makes it easy to test the resize/DPR bug fix across all 16+ test scenarios:
Architecture Highlights
.mount()functions, no React in call stackexamples-react/directoryTechnical Details
beforeId: 'watername_ocean'for MapLibre andslot: 'middle'for MapboxNote
Introduces a new
examples/basemap-browserapp for quickly exercising deck.gl with multiple basemaps, frameworks, and rendering modes.google-maps,mapbox, andmaplibre(withglobe) using sharedtypes,constants, andlayersinterleavedmode, and display live DPR/canvas/drawing buffer metrics (polled at 100ms).mount(...)helpers; React mounts as separate root; robust cleanup when switching examplesslot: 'middle'(Mapbox) orbeforeId: 'watername_ocean'(MapLibre)index.html,package.json(deps on deck.gl/map libs/react), andtsconfig.jsonto run locally viaviteWritten by Cursor Bugbot for commit 74b7215. This will update automatically on new commits. Configure here.