From 4295d9d733d2ae38a1abf11994af9963133911b0 Mon Sep 17 00:00:00 2001 From: Martin Schuhfuss Date: Mon, 5 Feb 2024 11:51:45 +0100 Subject: [PATCH] refactor: introduce initialCameraParams --- examples/multiple-maps/src/app.tsx | 1 - src/components/__tests__/map.test.tsx | 4 + src/components/map/index.tsx | 139 +++++------------- .../map/use-deckgl-camera-update.ts | 4 +- src/components/map/use-map-camera-params.ts | 27 ++-- src/components/map/use-map-events.ts | 12 +- src/components/map/use-map-instance.ts | 86 +++++++++++ .../map/use-tracked-camera-state-ref.ts | 84 +++++++---- src/libraries/lat-lng-utils.ts | 7 +- src/libraries/use-deep-compare-effect.tsx | 1 + src/libraries/use-force-update.ts | 7 + 11 files changed, 216 insertions(+), 156 deletions(-) create mode 100644 src/components/map/use-map-instance.ts create mode 100644 src/libraries/use-force-update.ts diff --git a/examples/multiple-maps/src/app.tsx b/examples/multiple-maps/src/app.tsx index fbeff05..1d02432 100644 --- a/examples/multiple-maps/src/app.tsx +++ b/examples/multiple-maps/src/app.tsx @@ -40,7 +40,6 @@ const App = () => { mapId={'49ae42fed52588c3'} disableDefaultUI onCameraChanged={isActive ? handleCameraChange : undefined} - controlled={!isActive} onMouseover={() => setActiveMap(i)} {...cameraState}> ); diff --git a/src/components/__tests__/map.test.tsx b/src/components/__tests__/map.test.tsx index 823461a..2ec3169 100644 --- a/src/components/__tests__/map.test.tsx +++ b/src/components/__tests__/map.test.tsx @@ -42,6 +42,10 @@ beforeEach(() => { super(...args); } }; + + // no idea why the implementation in @googlemaps/jest-mocks doesn't work as it is, + // but this helps: + google.maps.event.addListener = jest.fn(() => ({remove: jest.fn()})); }); afterEach(() => { diff --git a/src/components/map/index.tsx b/src/components/map/index.tsx index 9a04eb9..0070f1e 100644 --- a/src/components/map/index.tsx +++ b/src/components/map/index.tsx @@ -2,19 +2,14 @@ import React, { CSSProperties, PropsWithChildren, - Ref, useContext, useEffect, useLayoutEffect, - useMemo, - useState + useMemo } from 'react'; -import {APIProviderContext, APIProviderContextValue} from '../api-provider'; +import {APIProviderContext} from '../api-provider'; -import {useApiIsLoaded} from '../../hooks/use-api-is-loaded'; -import {logErrorOnce} from '../../libraries/errors'; -import {useCallbackRef} from '../../libraries/use-callback-ref'; import {MapEventProps, useMapEvents} from './use-map-events'; import {useMapOptions} from './use-map-options'; import {useTrackedCameraStateRef} from './use-tracked-camera-state-ref'; @@ -24,9 +19,10 @@ import { DeckGlCompatProps, useDeckGLCameraUpdate } from './use-deckgl-camera-update'; -import {isLatLngLiteral} from '../../libraries/is-lat-lng-literal'; +import {toLatLngLiteral} from '../../libraries/lat-lng-utils'; import {useMapCameraParams} from './use-map-camera-params'; import {AuthFailureMessage} from './auth-failure-message'; +import {useMapInstance} from './use-map-instance'; export interface GoogleMapsContextValue { map: google.maps.Map | null; @@ -44,8 +40,8 @@ export type { export type MapCameraProps = { center: google.maps.LatLngLiteral; zoom: number; - heading: number; - tilt: number; + heading?: number; + tilt?: number; }; /** @@ -54,16 +50,35 @@ export type MapCameraProps = { export type MapProps = google.maps.MapOptions & MapEventProps & DeckGlCompatProps & { + /** + * An id for the map, is required when multiple maps are present in the same APIProvider context. + */ id?: string; + /** + * Additional style rules to apply to the map dom-element. + */ style?: CSSProperties; + /** + * Additional css class-name to apply to the element containing the map. + */ className?: string; - initialBounds?: google.maps.LatLngBounds | google.maps.LatLngBoundsLiteral; + /** + * Indicates that the map will be controlled externally. Disables all controls provided by the map itself. + */ controlled?: boolean; + /** + * The initial parameters for the camera. If specified, the map will be in uncontrolled mode and + * will ignore the regular camera parameters (center/zoom/heading/tilt). + */ + initialCameraProps?: MapCameraProps; + /** + * Alternative way to specify the initialCameraProps as geographic region that should be visible + */ + initialBounds?: google.maps.LatLngBounds | google.maps.LatLngBoundsLiteral; }; export const Map = (props: PropsWithChildren) => { - const {children, id, className, style, controlled = false} = props; - + const {children, id, className, style} = props; const context = useContext(APIProviderContext); const loadingStatus = useApiLoadingStatus(); @@ -74,15 +89,16 @@ export const Map = (props: PropsWithChildren) => { } const [map, mapRef] = useMapInstance(props, context); - const cameraStateRef = useTrackedCameraStateRef(); + const cameraStateRef = useTrackedCameraStateRef(map); useMapCameraParams(map, cameraStateRef, props); - useMapEvents(map, cameraStateRef, props); + useMapEvents(map, props); useMapOptions(map, props); const isDeckGlControlled = useDeckGLCameraUpdate(map, props); + const isControlledExternally = !!props.controlled; - // disable interactions with the map for controlled modes + // disable interactions with the map for externally controlled maps useEffect(() => { if (!map) return; @@ -93,8 +109,8 @@ export const Map = (props: PropsWithChildren) => { map.setOptions({disableDefaultUI: true}); } - // disable all control-inputs when map is controlled - if (isDeckGlControlled || controlled) { + // disable all control-inputs when the map is controlled externally + if (isDeckGlControlled || isControlledExternally) { map.setOptions({ gestureHandling: 'none', keyboardShortcuts: false @@ -110,18 +126,13 @@ export const Map = (props: PropsWithChildren) => { }, [ map, isDeckGlControlled, - controlled, + isControlledExternally, props.gestureHandling, props.keyboardShortcuts ]); - // in controlled mode, any change to the camera state that isn't reflected in the props has to be prevented - const center = props.center - ? isLatLngLiteral(props.center) - ? props.center - : props.center.toJSON() - : null; - + // setup a stable cameraOptions object that can be used as dependency + const center = props.center ? toLatLngLiteral(props.center) : null; let lat: number | null = null; let lng: number | null = null; if (center && Number.isFinite(center.lat) && Number.isFinite(center.lng)) { @@ -138,9 +149,9 @@ export const Map = (props: PropsWithChildren) => { }; }, [lat, lng, props.zoom, props.heading, props.tilt]); - // controlled mode: reject all camera changes that don't correspond to changes in props + // externally controlled mode: reject all camera changes that don't correspond to changes in props useLayoutEffect(() => { - if (!map || !controlled) return; + if (!map || !isControlledExternally) return; map.moveCamera(cameraOptions); const listener = map.addListener('bounds_changed', () => { @@ -148,7 +159,7 @@ export const Map = (props: PropsWithChildren) => { }); return () => listener.remove(); - }, [map, controlled, cameraOptions]); + }, [map, isControlledExternally, cameraOptions]); const combinedStyle: CSSProperties = useMemo( () => ({ @@ -188,73 +199,3 @@ export const Map = (props: PropsWithChildren) => { ); }; Map.deckGLViewProps = true; - -/** - * The main hook takes care of creating map-instances and registering them in - * the api-provider context. - * @return a tuple of the map-instance created (or null) and the callback - * ref that will be used to pass the map-container into this hook. - * @internal - */ -function useMapInstance( - props: MapProps, - context: APIProviderContextValue -): readonly [map: google.maps.Map | null, containerRef: Ref] { - const apiIsLoaded = useApiIsLoaded(); - const [map, setMap] = useState(null); - const [container, containerRef] = useCallbackRef(); - - const { - id, - initialBounds, - - ...mapOptions - } = props; - - // create the map instance and register it in the context - useEffect( - () => { - if (!container || !apiIsLoaded) return; - - const {addMapInstance, removeMapInstance} = context; - const newMap = new google.maps.Map(container, mapOptions); - - setMap(newMap); - addMapInstance(newMap, id); - - if (initialBounds) { - newMap.fitBounds(initialBounds); - } - - return () => { - if (!container || !apiIsLoaded) return; - - // remove all event-listeners to minimize memory-leaks - google.maps.event.clearInstanceListeners(newMap); - - setMap(null); - removeMapInstance(id); - }; - }, - - // eslint-disable-next-line react-hooks/exhaustive-deps - [id, container, apiIsLoaded, props.mapId] - ); - - // report an error if the same map-id is used multiple times - useEffect(() => { - if (!id) return; - - const {mapInstances} = context; - - if (mapInstances[id] && mapInstances[id] !== map) { - logErrorOnce( - `The map id '${id}' seems to have been used multiple times. ` + - 'This can lead to unexpected problems when accessing the maps. ' + - 'Please use unique ids for all components.' - ); - } - }, [id, context, map]); - - return [map, containerRef] as const; -} diff --git a/src/components/map/use-deckgl-camera-update.ts b/src/components/map/use-deckgl-camera-update.ts index 781e0b4..6124a12 100644 --- a/src/components/map/use-deckgl-camera-update.ts +++ b/src/components/map/use-deckgl-camera-update.ts @@ -1,4 +1,4 @@ -import {useLayoutEffect, useMemo} from 'react'; +import {useLayoutEffect} from 'react'; export type DeckGlCompatProps = { /** @@ -24,7 +24,7 @@ export function useDeckGLCameraUpdate( props: DeckGlCompatProps ) { const {viewport, viewState} = props; - const isDeckGlControlled = useMemo(() => Boolean(viewport), [viewport]); + const isDeckGlControlled = !!viewport; useLayoutEffect(() => { if (!map || !viewState) return; diff --git a/src/components/map/use-map-camera-params.ts b/src/components/map/use-map-camera-params.ts index 6a6d41c..94e52f3 100644 --- a/src/components/map/use-map-camera-params.ts +++ b/src/components/map/use-map-camera-params.ts @@ -1,18 +1,14 @@ import {useLayoutEffect} from 'react'; import {CameraStateRef} from './use-tracked-camera-state-ref'; -import {MapProps} from '@vis.gl/react-google-maps'; -import {isLatLngLiteral} from '../../libraries/is-lat-lng-literal'; +import {toLatLngLiteral} from '../../libraries/lat-lng-utils'; +import {MapProps} from '../map'; export function useMapCameraParams( map: google.maps.Map | null, cameraStateRef: CameraStateRef, mapProps: MapProps ) { - const center = mapProps.center - ? isLatLngLiteral(mapProps.center) - ? mapProps.center - : mapProps.center.toJSON() - : null; + const center = mapProps.center ? toLatLngLiteral(mapProps.center) : null; let lat: number | null = null; let lng: number | null = null; @@ -32,15 +28,18 @@ export function useMapCameraParams( ? (mapProps.tilt as number) : null; - /* eslint-disable react-hooks/exhaustive-deps -- - * - * The following effects aren't triggered when the map is changed. - * In that case, the values will be or have been passed to the map - * constructor via mapOptions. - */ + // the following effect runs for every render of the map component and checks + // if there are differences between the known state of the map instance + // (cameraStateRef, which is updated by all bounds_changed events) and the + // desired state in the props. + useLayoutEffect(() => { if (!map) return; + // when the map was configured with an initialCameraProps instead + // of center/zoom/..., we skip all camera updates here + if (mapProps.initialCameraProps) return; + const nextCamera: google.maps.CameraOptions = {}; let needsUpdate = false; @@ -72,5 +71,5 @@ export function useMapCameraParams( if (needsUpdate) { map.moveCamera(nextCamera); } - }, [cameraStateRef, lat, lng, zoom, heading, tilt]); + }); } diff --git a/src/components/map/use-map-events.ts b/src/components/map/use-map-events.ts index 66dddc4..2ae94c1 100644 --- a/src/components/map/use-map-events.ts +++ b/src/components/map/use-map-events.ts @@ -1,8 +1,4 @@ import {useEffect} from 'react'; -import { - CameraStateRef, - trackDispatchedEvent -} from './use-tracked-camera-state-ref'; /** * Handlers for all events that could be emitted by map-instances. @@ -45,7 +41,6 @@ export type MapEventProps = Partial<{ */ export function useMapEvents( map: google.maps.Map | null, - cameraStateRef: CameraStateRef, props: MapEventProps ) { // note: calling a useEffect hook from within a loop is prohibited by the @@ -68,15 +63,12 @@ export function useMapEvents( map, eventType, (ev?: google.maps.MapMouseEvent | google.maps.IconMouseEvent) => { - const mapEvent = createMapEvent(eventType, map, ev); - trackDispatchedEvent(mapEvent, cameraStateRef); - - handler(mapEvent); + handler(createMapEvent(eventType, map, ev)); } ); return () => listener.remove(); - }, [map, cameraStateRef, eventType, handler]); + }, [map, eventType, handler]); } } diff --git a/src/components/map/use-map-instance.ts b/src/components/map/use-map-instance.ts new file mode 100644 index 0000000..7265308 --- /dev/null +++ b/src/components/map/use-map-instance.ts @@ -0,0 +1,86 @@ +import {Ref, useEffect, useState} from 'react'; + +import {MapProps} from '../map'; +import {APIProviderContextValue} from '../api-provider'; + +import {useCallbackRef} from '../../libraries/use-callback-ref'; +import {useApiIsLoaded} from '../../hooks/use-api-is-loaded'; + +/** + * The main hook takes care of creating map-instances and registering them in + * the api-provider context. + * @return a tuple of the map-instance created (or null) and the callback + * ref that will be used to pass the map-container into this hook. + * @internal + */ +export function useMapInstance( + props: MapProps, + context: APIProviderContextValue +): readonly [map: google.maps.Map | null, containerRef: Ref] { + const apiIsLoaded = useApiIsLoaded(); + const [map, setMap] = useState(null); + const [container, containerRef] = useCallbackRef(); + + const { + id, + initialBounds, + initialCameraProps, + + ...mapOptions + } = props; + + // create the map instance and register it in the context + useEffect( + () => { + if (!container || !apiIsLoaded) return; + + const {addMapInstance, removeMapInstance} = context; + const newMap = new google.maps.Map(container, mapOptions); + + setMap(newMap); + addMapInstance(newMap, id); + + if (initialBounds) { + newMap.fitBounds(initialBounds); + } + + if (initialCameraProps) { + newMap.setOptions(initialCameraProps); + } + + return () => { + if (!container || !apiIsLoaded) return; + + // remove all event-listeners to minimize memory-leaks + google.maps.event.clearInstanceListeners(newMap); + + setMap(null); + removeMapInstance(id); + }; + }, + + // some dependencies are ignored in the list below: + // - initialBounds and initialCameraProps will only be used once, and + // changes should be ignored + // - mapOptions has special hooks that take care of updating the options + // eslint-disable-next-line react-hooks/exhaustive-deps + [container, apiIsLoaded, id, props.mapId] + ); + + // report an error if the same map-id is used multiple times + // useEffect(() => { + // if (!id) return; + // + // const {mapInstances} = context; + // + // if (mapInstances[id] && mapInstances[id] !== map) { + // logErrorOnce( + // `The map id '${id}' seems to have been used multiple times. ` + + // 'This can lead to unexpected problems when accessing the maps. ' + + // 'Please use unique ids for all components.' + // ); + // } + // }, [id, context, map]); + + return [map, containerRef] as const; +} diff --git a/src/components/map/use-tracked-camera-state-ref.ts b/src/components/map/use-tracked-camera-state-ref.ts index 7d9b99a..d3c8bdc 100644 --- a/src/components/map/use-tracked-camera-state-ref.ts +++ b/src/components/map/use-tracked-camera-state-ref.ts @@ -1,5 +1,5 @@ -import {MutableRefObject, useRef} from 'react'; -import {MapCameraChangedEvent, MapEvent} from './use-map-events'; +import {MutableRefObject, useEffect, useRef} from 'react'; +import {useForceUpdate} from '../../libraries/use-force-update'; export type CameraState = { center: google.maps.LatLngLiteral; @@ -10,37 +10,69 @@ export type CameraState = { export type CameraStateRef = MutableRefObject; +function handleBoundsChange(map: google.maps.Map, ref: CameraStateRef) { + const center = map.getCenter(); + const zoom = map.getZoom(); + const heading = map.getHeading() || 0; + const tilt = map.getTilt() || 0; + const bounds = map.getBounds(); + + if (!center || !bounds || !Number.isFinite(zoom)) { + console.warn( + '[useTrackedCameraState] at least one of the values from the map ' + + 'returned undefined. This is not expected to happen. Please ' + + 'report an issue at https://github.com/visgl/react-google-maps/issues/new' + ); + } + + // fixme: do we need the `undefined` cases for the camera-params? When are they used in the maps API? + Object.assign(ref.current, { + center: center?.toJSON() || {lat: 0, lng: 0}, + zoom: (zoom as number) || 0, + heading: heading as number, + tilt: tilt as number + }); +} + /** * Creates a mutable ref object to track the last known state of the map camera. - * This is updated by `trackDispatchedEvent` and used in `useMapOptions`. + * This is used in `useMapCameraParams` to reduce stuttering in normal operation + * by avoiding updates of the map camera with values that have already been processed. */ -export function useTrackedCameraStateRef(): CameraStateRef { - return useRef({ +export function useTrackedCameraStateRef( + map: google.maps.Map | null +): CameraStateRef { + const forceUpdate = useForceUpdate(); + const ref = useRef({ center: {lat: 0, lng: 0}, heading: 0, tilt: 0, zoom: 0 }); -} -/** - * Records camera data from the last event dispatched to the React application. - * This data can then be used to prevent feeding these values back to the - * map-instance when a typical "controlled component" setup (state variable is - * fed into and updated by the map). - */ -export function trackDispatchedEvent( - ev: MapEvent, - cameraStateRef: CameraStateRef -) { - const cameraEvent = ev as MapCameraChangedEvent; - - // we're only interested in the camera-events here - if (!cameraEvent.detail.center) return; - const {center, zoom, heading, tilt} = cameraEvent.detail; - - cameraStateRef.current.center = center; - cameraStateRef.current.heading = heading; - cameraStateRef.current.tilt = tilt; - cameraStateRef.current.zoom = zoom; + // Record camera state with every bounds_changed event dispatched by the map. + // This data is used to prevent feeding these values back to the + // map-instance when a typical "controlled component" setup (state variable is + // fed into and updated by the map). + useEffect(() => { + if (!map) return; + + const listener = google.maps.event.addListener( + map, + 'bounds_changed', + () => { + handleBoundsChange(map, ref); + + // When an event is occured, we have to update during the next cycle. + // The application could decide to ignore the event and not update any + // camera props of the map, meaning that in that case we will have to + // 'undo' the change to the camera. + forceUpdate(); + } + ); + + return () => listener.remove(); + }, [map, forceUpdate]); + + return ref; } diff --git a/src/libraries/lat-lng-utils.ts b/src/libraries/lat-lng-utils.ts index e8e8be6..844a32f 100644 --- a/src/libraries/lat-lng-utils.ts +++ b/src/libraries/lat-lng-utils.ts @@ -21,8 +21,7 @@ export function latLngEquals( export function toLatLngLiteral( obj: google.maps.LatLngLiteral | google.maps.LatLng ): google.maps.LatLngLiteral { - return { - lat: typeof obj.lat === 'function' ? obj.lat() : obj.lat, - lng: typeof obj.lng === 'function' ? obj.lng() : obj.lng - }; + if (isLatLngLiteral(obj)) return obj; + + return obj.toJSON(); } diff --git a/src/libraries/use-deep-compare-effect.tsx b/src/libraries/use-deep-compare-effect.tsx index 9d619e7..c2d8402 100644 --- a/src/libraries/use-deep-compare-effect.tsx +++ b/src/libraries/use-deep-compare-effect.tsx @@ -11,5 +11,6 @@ export function useDeepCompareEffect( ref.current = deps; } + // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(effect, ref.current); } diff --git a/src/libraries/use-force-update.ts b/src/libraries/use-force-update.ts new file mode 100644 index 0000000..c3ba9f5 --- /dev/null +++ b/src/libraries/use-force-update.ts @@ -0,0 +1,7 @@ +import {useReducer} from 'react'; + +export function useForceUpdate(): () => void { + const [, forceUpdate] = useReducer(x => x + 1, 0); + + return forceUpdate; +}