Skip to content

Commit

Permalink
feat: cleanup map, remove onLoadMap prop
Browse files Browse the repository at this point in the history
BREAKING CHANGE: removed MapProps.onLoadMap
  • Loading branch information
usefulthink committed Nov 9, 2023
1 parent 0b1d800 commit d5e7dfd
Showing 1 changed file with 5 additions and 33 deletions.
38 changes: 5 additions & 33 deletions src/components/map.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import React, {
CSSProperties,
PropsWithChildren,
Ref,
RefCallback,
useCallback,
useContext,
useEffect,
useLayoutEffect,
Expand All @@ -18,7 +16,6 @@ import {useApiIsLoaded} from '../hooks/use-api-is-loaded';
import {logErrorOnce} from '../libraries/errors';
import {useCallbackRef} from '../libraries/use-callback-ref';

// Google Maps context
export interface GoogleMapsContextValue {
map: google.maps.Map | null;
}
Expand All @@ -45,10 +42,6 @@ export type MapProps = google.maps.MapOptions & {
* This is also needed to reference the map inside the useMap hook.
*/
id?: string;
/**
* A callback function that is called, when the Google Map is loaded.
*/
onLoadMap?: (map: google.maps.Map) => void;
/**
* Viewport from deck.gl
*/
Expand Down Expand Up @@ -129,69 +122,48 @@ function useMapInstanceHandlerEffects(
const {
id,
initialBounds,
onLoadMap,
// FIXME: this destructuring here leads to mapOptions being a new
// object for every render, and we'll be calling map.setOptions() a lot.

...mapOptions
} = props;

// create the map instance and register it in the context
useEffect(
() => {
// this will be called a couple of times before the dependencies are
// actually ready to create the map
if (!container || !apiIsLoaded) return;

// Since we can't know the map-ids used in sibling components during
// rendering, we can't check for existing maps with the same id here.
// We do have a seperate hook below that keeps an eye on mapIds and will
// write an error-message to the console if reused ids are detected.
const {addMapInstance, removeMapInstance} = context;
const newMap = new google.maps.Map(container, mapOptions);
setMap(newMap);
addMapInstance(newMap, id);

if (onLoadMap) {
google.maps.event.addListenerOnce(newMap, 'idle', () => {
onLoadMap(newMap);
});
}

if (initialBounds) {
newMap.fitBounds(initialBounds);
}

return () => {
if (!container || !apiIsLoaded) return;

// remove all event-listeners to minimize memory-leaks
google.maps.event.clearInstanceListeners(container);

setMap(null);
removeMapInstance(id);
};
},

// Dependencies need to be inaccurately limited here. The cleanup function
// will remove the map-instance with all its internal state, and we can't
// have that happening. This is only ok when the id or mapId is changed,
// since this requires a new map to be created anyway.

// FIXME: we should rethink if it could be possible to keep the state
// around when a map gets re-initialized (id or mapId changed). This
// should keep the viewport as it is (so also no initial viewport in
// this case) and any added features should of course stay as well (though
// that is for those components to figure out to always be attached to
// the correct map-instance from the context) .
// this case) and any added features should of course get re-added as
// well.

// 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;
}
if (!id) return;

const {mapInstances} = context;

Expand Down

0 comments on commit d5e7dfd

Please sign in to comment.