visgl / react-map-gl

React friendly API wrapper around MapboxGL JS
http://visgl.github.io/react-map-gl/
Other
7.71k stars 1.33k forks source link

[Bug] Invalid type: 'container' must be a String or HTMLElement. #1960

Open Jamie5 opened 1 year ago

Jamie5 commented 1 year ago

Description

We get an error "Invalid type: 'container' must be a String or HTMLElement." under certain circumstances which are flaky and unclear. The stack trace points to react-map-gl, but this is using a error logging system and local reproduction has not proven successful.

Here is my theory as to what is happening (note: it was difficult to find documentation regarding the React lifecycle as it pertains to what is mentioned below):

  1. The Map mounts, which kicks off the library load, and that is in progress
  2. Now, Map begins an unmount for whatever reason.
  3. containerRef.current gets set to null as part of the unmount
  4. The library load finishes, and hence the rest of the promise runs. It checks isMounted, which is still set to true, and hence continues running. It runs mapbox = new Mapbox(mapboxgl.Map, props, containerRef.current);, which fails with the above error as containerRef.current is null.
  5. The useEffect cleanup function runs and sets isMounted to false, which would have made the promise stop running, but alas too late.

Now whether this theory works depends on at least two things

  1. Refs getting nulled out before useEffect cleanup is run
  2. async code being able to run between the ref clearing and useEffect cleanup happening

I don't know about the second point, but an experiment with React (17.0.2) roughly as follows

  useEffect(() => {
    console.log('XXX USE EFFECT');
    return () => {
      console.log('XXX USE EFFECT CLEANUP');
    }
  }, []);

  return <div ref={(item) => console.log('XXX SET REF', item)} />;

prints

which seems to support that refs are cleared before useEffect cleanups are run.

Assuming this theory seems plausible, it perhaps would be better to check for the presence of containerRef.current in addition to the isMounted check. (from what I can tell, containerRef is guaranteed to be set from the first render before useEffect runs, so long as it doesn't get cleared by an unmount)

Expected Behavior

No error message

Steps to Reproduce

Unknown :(

Environment

Logs

No response

anna-visarsoft commented 1 year ago

I have the same issue.

brncsk commented 1 year ago

Happens to me as well when the map is inside a <Suspense> container and is being suspended.

anna-visarsoft commented 1 year ago

I have fixed this issue. In my case it was my mistake. I tried to render map with markers until got all markers id. So system tried to add marker with empty id '' and it caused this error. So adding condition for map rendering helped me.

  const [farmMarker, setFarmMarker] = useState({
    id: "",
    lat: 0,
    lng: 0,
  });
  ...
useEffect(() => {
    const result = await getAsync(
      API_FARM_DETAILS(companyId, farmResult.externalId)
    );
    setFarmMarker((fm) => {
      return {
        ...fm,
        id: result.externalId,
        lat: result.latitude,
        lng: result.longitude,
      };
    });
}, [...]);

  ...
  {farmMarker.id && <AquaMapBoxGl
    position={mapPosition}
    markers={[farmMarker]}
    selectedMarkerId={farmMarker.id}
    onMarkerClick={() => {}}
  />}
benzara-tahar commented 1 year ago

any updates on this issue?

straube commented 1 year ago

I'm facing the same issue. I debugged the code in Chrome's Dev Tools and I found out that sometimes containerRef.current can be null the first time the useEffect hook runs (https://github.com/visgl/react-map-gl/blob/7.0-release/src/components/map.tsx#L103).

If a page contains only the map it usually works fine. But on a page with a more complex structure (a bunch of other components that take more time to render), the error appears in the browser console.

A possible fix would be checking if the actual container ref (a <div /> element) was assigned to containerRef before instancing the Mapbox class. Example:

if (!(isMounted && containerRef.current)) {
    return;
}

I guess the dependency array for that hook would have to change from [] to [ containerRef ] to make sure it runs again when the ref changes. I'm not sure what would be all the possible side effects of simply doing that, though.

chrisgervang commented 1 year ago

I ran into this as well when quickly mounting and unmounting the map before it fully initializes. @Pessimistress what do you think of @straube's approach? I was thinking along the same lines.

andreu86 commented 11 months ago

Any news about this issue? Thx

chrisgervang commented 10 months ago

I couldn't reproduce with this basic example. We need a reproduction in an isolated example to determine if it's an issue with react-map-gl. Any ideas?

Screen Shot 2023-09-12 at 11 50 59 AM
eatyourgreens commented 3 weeks ago

We've run into this error with a map that renders inside a Suspense boundary, but only when using the concurrent rendering API in React 18. Rolling back to reactDOM.render() seems to be a workaround.

Pessimistress commented 3 weeks ago

@eatyourgreens can you provide a setup that we can use to reproduce the issue?

eatyourgreens commented 3 weeks ago

@Pessimistress Here's the setup that crashes for us:

<Suspense fallback={null}>
  <Map
      reuseMaps={true}
      styleDiffing={true}
      {...viewState}
      onMove={({ viewState }) => onViewState(viewState)}
      mapStyle={mapStyle}
      dragRotate={false}
      keyboard={false}
      touchZoomRotate={true}
      touchPitch={false}
      antialias={true}
      attributionControl={false}
    >
      {children}
  </Map>
</Suspense>

Moving the Map outside the Suspense boundary might fix it for us. See https://github.com/nismod/irv-jamaica/pull/42.

chrisgervang commented 3 weeks ago

Could you please hook this up in a Codesandbox, CodePen (somewhere with your lib setup running)? Thanks, @eatyourgreens.

eatyourgreens commented 3 weeks ago

@chrisgervang I tried removing {children} from my map component, to set up a simple reproduction on codepen, and the bug went away. In our case, the Map component wraps a bunch of lazy-loaded data layers, and those seem to be causing the problem in React 18. I think maybe Suspense is waiting for all of the children to load before rendering Map, and the container ref used by React Map GL is null while the children load.

This worked in our code with React 17, but breaks now I've upgraded to React 18:

<Suspense fallback={null}>
  <Map>
    <LazyLoadedDeckGLLayers />
  </Map>
</Suspense>

This seems to work in React 18:

<Map>
  <Suspense fallback={null}>
    <LazyLoadedDeckGLLayers />
  </Suspense>
</Map>

I'll see if I can set up a sandbox example of the broken setup.

EDIT: I isolated the change that introduced the bug into its own commit. It was just changing the app over from reactDOM.render() to createRoot().render(). I think that may have changed how the Suspense boundary is handled. https://github.com/nismod/irv-jamaica/commit/2850a4fb453a0339f2f92b5e00b1610afd98887b