fix: use latest reuseMaps value in map instance cleanup (#1042)

reuseMaps was read directly from the effect's closure, but the effect
intentionally excludes it from its dependency array (toggling it alone
shouldn't recreate the map). As a result, if reuseMaps changed between
mount and unmount, the cleanup used the stale value captured at mount,
either leaking an instance onto the static cache stack or discarding
one that should have been cached. Track it in a ref that's kept current
via its own effect, and read that ref in both the create and cleanup
paths instead.

---------

Co-authored-by: Martin Schuhfuss <m.schuhfuss@gmail.com>

Author: zigzagdev

src/components/__tests__/map.test.tsx

@@ -44,6 +44,7 @@ beforeEach(() => {
     constructor(...args: ConstructorParameters<typeof google.maps.Map>) {
       createMapSpy(...args);
       super(...args);
+      this.getDiv = jest.fn().mockImplementation(() => args[0]);
     }
   };
 
@@ -245,6 +246,97 @@ describe('map instance caching', () => {
     // a fresh map instance should have been created instead of reusing the broken one
     expect(createMapSpy).toHaveBeenCalled();
   });
+
+  test('respects reuseMaps being turned off before unmount, even without other prop changes', async () => {
+    // mapId/renderingType/colorScheme stay constant, so the effect that reads
+    // reuseMaps is intentionally not re-run when only reuseMaps changes. The
+    // cleanup must still see the latest reuseMaps value instead of the one
+    // captured when the map was created: with reuseMaps now false, unmounting
+    // must clear the instance's listeners instead of pushing it onto the
+    // internal (unbounded, never-reused) cache stack.
+    const center = {lat: 53.55, lng: 10.05};
+
+    const {rerender, unmount} = render(
+      <GoogleMap mapId={'toggle-reuse'} reuseMaps center={center} zoom={12} />,
+      {wrapper}
+    );
+    await waitFor(() => expect(screen.getByTestId('map')).toBeInTheDocument());
+    const mapInstance = mockInstances.get(google.maps.Map).at(-1)!;
+
+    rerender(
+      <GoogleMap
+        mapId={'toggle-reuse'}
+        reuseMaps={false}
+        center={center}
+        zoom={12}
+      />
+    );
+
+    // note: checking for clearInstanceListeners being called is an implementation
+    // detail that shouldn't be in this test, but it's the only way we can tell
+    // if the map instance was discarded or kept around.
+    jest.mocked(google.maps.event.clearInstanceListeners).mockClear();
+    unmount();
+
+    expect(google.maps.event.clearInstanceListeners).toHaveBeenCalledWith(
+      mapInstance
+    );
+  });
+
+  test('respects reuseMaps being turned on before unmount, even without other prop changes', async () => {
+    // mapId/renderingType/colorScheme stay constant, so the effect that reads
+    // reuseMaps is intentionally not re-run when only reuseMaps changes. The
+    // cleanup must still see the latest reuseMaps value instead of the one
+    // captured when the map was created: with reuseMaps now true, unmounting
+    // must push the instance onto the cache stack so it can be reused on remount.
+    const center = {lat: 53.55, lng: 10.05};
+
+    const {rerender, unmount} = render(
+      <GoogleMap
+        mapId={'toggle-reuse-on'}
+        reuseMaps={false}
+        center={center}
+        zoom={12}
+      />,
+      {wrapper}
+    );
+    await waitFor(() => expect(screen.getByTestId('map')).toBeInTheDocument());
+    const mapInstance = mockInstances.get(google.maps.Map).at(-1)!;
+
+    rerender(
+      <GoogleMap
+        mapId={'toggle-reuse-on'}
+        reuseMaps
+        center={center}
+        zoom={12}
+      />
+    );
+
+    // note: checking for clearInstanceListeners being called is an implementation
+    // detail that shouldn't be in this test, but it's the only way we can tell
+    // if the map instance was discarded or kept around.
+    jest.mocked(google.maps.event.clearInstanceListeners).mockClear();
+    unmount();
+
+    expect(google.maps.event.clearInstanceListeners).not.toHaveBeenCalledWith(
+      mapInstance
+    );
+
+    createMapSpy.mockClear();
+    render(
+      <GoogleMap
+        mapId={'toggle-reuse-on'}
+        reuseMaps
+        center={center}
+        zoom={12}
+      />,
+      {wrapper}
+    );
+    await waitFor(() => expect(screen.getByTestId('map')).toBeInTheDocument());
+
+    expect(createMapSpy).not.toHaveBeenCalled();
+    expect(mockInstances.get(google.maps.Map).at(-1)).toBe(mapInstance);
+  });
 });
 
 describe('camera configuration', () => {

src/components/map/use-map-instance.ts

@@ -124,6 +124,14 @@ export function useMapInstance(
     cameraState: CameraState;
   }>(undefined);
 
+  // kept up to date on every render so the effect below (which intentionally
+  // doesn't list reuseMaps as a dependency) always sees the latest value,
+  // both when creating a map and in its cleanup function on unmount.
+  const reuseMapsRef = useRef(reuseMaps);
+  useEffect(() => {
+    reuseMapsRef.current = reuseMaps;
+  }, [reuseMaps]);
+
   // create the map instance and register it in the context
   useEffect(
     () => {
@@ -143,7 +151,7 @@ export function useMapInstance(
       // correctly). In that case `getDiv()` doesn't return a usable DOM node,
       // so we have to discard the cached instance instead of trying to reuse it.
       const cachedMap =
-        reuseMaps && CachedMapStack.has(cacheKey)
+        reuseMapsRef.current && CachedMapStack.has(cacheKey)
           ? (CachedMapStack.pop(cacheKey) as google.maps.Map)
           : null;
       const cachedMapDiv = cachedMap?.getDiv();
@@ -181,7 +189,6 @@ export function useMapInstance(
         });
       }
 
-      // eslint-disable-next-line react-hooks/set-state-in-effect -- intentional to sync the imperative instance with state
       setMap(map);
       addMapInstance(map, id);
 
@@ -214,7 +221,7 @@ export function useMapInstance(
         // detach the map-div from the dom
         mapDiv.remove();
 
-        if (reuseMaps) {
+        if (reuseMapsRef.current) {
           // push back on the stack
           CachedMapStack.push(cacheKey, map);
         } else {