[Bug] On clicking AdvanceMarker to open InfoWindow, goes into an Infinite loop of rendering on setting markerRef(marker) manually

[m-nathani]

Description

I am using the Cluster example the docs with MarkerWithInfoWindow, however there is nothing much i changed for cluster or infowindow from the docs... onClick on the marker should have open the infowindow, but instead to goes in an infinite loop.

Please note that i am setting the markerRef(marker); on the ref of AdvanceMarker manually because i want to support Cluster along with it... it appears to be a standard way to me to set ref to multiple places for marker... however it crashes

const MarkerWithInfoWindow = forwardRef<
  AdvancedMarkerRef,
  AdvancedMarkerProps & {
    point: Point;
    setMarkerRef: (marker: Marker | null, key: number) => void;
  }
>(({ point, setMarkerRef, ...props }, ref) => {
  let [markerRef, marker] = useAdvancedMarkerRef();
  const [infoWindowOpen, setInfoWindowOpen] = useState(false);

  return (
    <>
      <AdvancedMarker
        ref={(marker1) => {
          markerRef(marker1);
          setMarkerRef(marker1, point.key);
        }}
        onClick={(e) => {
          setInfoWindowOpen(true);
        }}
        position={point}
        title={point.name}
        zIndex={50}
        {...props}
      />
      {infoWindowOpen && (
        <InfoWindow anchor={marker} maxWidth={200} onCloseClick={() => setInfoWindowOpen(false)}>
          <code style={{ whiteSpace: 'nowrap' }}>&lt;AdvancedMarker /&gt;</code> combined with an
          Infowindow.
        </InfoWindow>
      )}
    </>
  );
});

Steps to Reproduce

I am using next 14, and this is a client component.

1. Click on the marker and it goes on an infinite loop and hence the app crashes.

Environment

• Library version: "@vis.gl/react-google-maps": "^1.0.2",
• Google maps version: 3.57.3a
• Browser and Version: Chrome 125.0.6422.141 (Official Build) (64-bit)
• OS: Linux ( Ubuntu )

Logs

[usefulthink]

I think I have an Idea whats happening:

1. rendering this component will eventually invoke the ref-callback of the AdvancedMarker
2. via markerRef() this will update the marker state-variable, causing the component to re-render
3. when re-rendering, the ref- and onClick- props of the AdvancedMarker are changed, so the AM needs to re-render as well
4. this causes the ref to be updated, so we're back at step 1.

To mitigate, it should be enough to memoize the two callbacks you're passing to AdvancedMarker, which is a good practice anyway...

const MarkerWithInfoWindow = forwardRef<
  AdvancedMarkerRef,
  AdvancedMarkerProps & {
    point: Point;
    setMarkerRef: (marker: Marker | null, key: number) => void;
  }
>(({ point, setMarkerRef, ...props }, ref) => {
  let [markerRef, marker] = useAdvancedMarkerRef();
  const [infoWindowOpen, setInfoWindowOpen] = useState(false);

  const refCallback = useCallback((marker1) => {
    markerRef(marker1);
    setMarkerRef(marker1, point.key);
  }, [setMarkerRef, point.key]);

  const handleClick = useCallback((e) => {
    setInfoWindowOpen(true);
  }, []);

  return (
    <>
      <AdvancedMarker
        ref={refCallback}
        onClick={handleClick}
        position={point}
        title={point.name}
        zIndex={50}
        {...props}
      />
      {infoWindowOpen && (
        <InfoWindow anchor={marker} maxWidth={200} onCloseClick={() => setInfoWindowOpen(false)}>
          <code style={{ whiteSpace: 'nowrap' }}>&lt;AdvancedMarker /&gt;</code> combined with an
          Infowindow.
        </InfoWindow>
      )}
    </>
  );
});

[m-nathani]

Great, i will check on it.. however its a bit of confusion that a ref update can cause re-render, since in react, refs are global to the component lifecycle and updating it should not re-render the component.

Let me know if there misunderstanding on implementation of ref or how it should work ?

[usefulthink]

This case is slightly different – since we need to react to changing of the ref (update the anchor of the infowindow when the marker API-element is available), useAdvancedMarkerRef uses the ref-callback format together with a state-variable signalling changing of the property and triggering a re-render. Normally this isn't a problem since the callback is memoized and doesn't trigger a re-render by itself. In your case, when the ref-callback isn't memoized, you'll potentially run into problems.

I'm always open to suggestions how to architect this differently, but as far as I am aware this is the best solution we have available to build the connection of the marker to the infowindow.

[m-nathani]

This case is slightly different – since we need to react to changing of the ref (update the anchor of the infowindow when the marker API-element is available), useAdvancedMarkerRef uses the ref-callback format together with a state-variable signalling changing of the property and triggering a re-render. Normally this isn't a problem since the callback is memoized and doesn't trigger a re-render by itself. In your case, when the ref-callback isn't memoized, you'll potentially run into problems.

I'm always open to suggestions how to architect this differently, but as far as I am aware this is the best solution we have available to build the connection of the marker to the infowindow.

ah i see...

This code is not directly related with ref.. it's just saving into state so that can be reused for infowindow anchor:

export function useAdvancedMarkerRef() {
  const [marker, setMarker] =
    useState<google.maps.marker.AdvancedMarkerElement | null>(null);

  const refCallback = useCallback((m: AdvancedMarkerRef | null) => {
    setMarker(m);
  }, []);

  return [refCallback, marker] as const;
}

the term ref in useAdvancedMarkerRef was a confusion, as i was in assumption that its using useRef or createRef underneath to store the marker which is exposed to infowindow.. However, to update the infowindow it needs useState to re-render.

Just my few cents:

Can we store marker in useRef and useState for open and close states... it's simple and reflect the terminology used.

const markerRef = useRef<google.maps.marker.AdvancedMarkerElement | null>(null); 
const [infoWindowOpen, setInfoWindowOpen] = useState(false);

const refCallback = useCallback((marker) => {
  markerRef.current = marker;
  setMarkerRef(marker, point.key);
}, [setMarkerRef, point.key]);

const handleClick = useCallback((e) => {
  setInfoWindowOpen(true);
}, []);

return (
  <>
    <AdvancedMarker
      ref={refCallback}
      onClick={handleClick}
      position={point}
      title={point.name}
      zIndex={50}
      {...props}
    />
    {infoWindowOpen && markerRef.current && (
      <InfoWindow anchor={markerRef.current} maxWidth={200} onCloseClick={() => setInfoWindowOpen(false)}>
        <code style={{ whiteSpace: 'nowrap' }}>&lt;AdvancedMarker /&gt;</code> combined with an
        Infowindow.
      </InfoWindow>
    )}
  </>
)

if you want to open the marker by default then use the useEffect for mount (with no deps) , to update setInfoWindowOpen(true); . 🤔

[usefulthink]

That's interesting to read and I understand the confusion. Originally, React only had the CallbackRef style refs, iirc the newer style (mutable object ref) that we now know as 'ref' was introduced together with hooks in 16.8, probably precisely because of the problems the callbacks can have. Still, both are valid formats for a ref.

Your suggestion will probably work as well, but you the component will not notice when the ref'd object (the google.maps.marker.AdvancedMarkerElement) is set. It will only work because you trigger a re-render by setting the infoWindowOpen state after the marker is already rendered (and thus the ref.current having a value). If, for whatever reason, this wasn't the case, the infowindow would have no anchor.

Also let me note that the documentation is pretty clear about not using ref.current during rendering:

Do not write or read ref.current during rendering.

React expects that the body of your component behaves like a pure function:

• If the inputs (props, state, and context) are the same, it should return exactly the same JSX.
• Calling it in a different order or with different arguments should not affect the results of other calls.

Reading or writing a ref during rendering breaks these expectations.

[m-nathani]

Ah great explanation, thanks for pointing out the best practice. 🙏🏼

However, this was a bit ambiguous to understand, usually it always works or in my experience on mounted there's DOM available.

If, for whatever reason, this wasn't the case, the infowindow would have no anchor.

[usefulthink]

just that: if the internal marker object in markerRef.current has – for whatever reason – not yet been created when this condition is evaluated:

{infoWindowOpen && markerRef.current && ( ... ) }

There'll be no re-render once it is available, so no way to recover from that.

[usefulthink]

I'll convert this to a discussion, it doesn't look like theres an actual bug in our library here.