Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Jamie5 opened this issue Aug 23, 2022 · 13 comments
Open

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

Jamie5 opened this issue Aug 23, 2022 · 13 comments
Labels

Comments

@Jamie5
Copy link
Contributor

Jamie5 commented Aug 23, 2022

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

  • XXX SET REF <div>
  • XXX USE EFFECT
    [navigate away]
  • XXX SET REF null
  • XXX USE EFFECT CLEANUP

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

@Jamie5 Jamie5 added the bug label Aug 23, 2022
@anna-visarsoft
Copy link

I have the same issue.

@brncsk
Copy link

brncsk commented Sep 22, 2022

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

@anna-visarsoft
Copy link

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
Copy link

any updates on this issue?

@straube
Copy link

straube commented May 10, 2023

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
Copy link
Contributor

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
Copy link

Any news about this issue? Thx

@chrisgervang
Copy link
Contributor

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
Copy link

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
Copy link
Collaborator

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

@eatyourgreens
Copy link

eatyourgreens commented Jun 24, 2024

@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 nismod/irv-jamaica#42.

@chrisgervang
Copy link
Contributor

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

@eatyourgreens
Copy link

eatyourgreens commented Jun 24, 2024

@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.
nismod/irv-jamaica@2850a4f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants