React-mapbox-gl: warning: in some cases setState() may be called after the unmount

[jbilcke]

This warning does happen from time to time:

Warning: setState(...): Can only update a mounted or mounting component. 
This usually means you called setState() on an unmounted component. 
This is a no-op. Please check the code for the ReactMapboxGl component.

The related code:

    map.on('load', (evt: React.SyntheticEvent<any>) => {
      this.setState({ map });

      if (onStyleLoad) {
        onStyleLoad(map, evt);
      }
    });

https://github.com/alex3165/react-mapbox-gl/blob/master/src/map.tsx#L181

If the ReactMapboxGl component is unmounted before load finishes, the callback will try to update the state, it does nothing but this is not recommended

calling setState() in an unmounted component means that your app 
is still holding a reference to the component after the component 
has been unmounted - which often indicates a memory leak

https://facebook.github.io/react/blog/2015/12/16/ismounted-antipattern.html

[lamuertepeluda]

I also faced this issue sometimes. I'm not really convinced about passing the map as state is the right way to go.
This brings to a potentially bigger issue: is it correct to use the React context API here? Wouldn't it be better to user redux + props?

Of course this would mean redesign the map as HOC and have a Redux state and all the map children such as Layer, Source, whatever... as connected components.

[gpbmike]

I believe the event listeners are being incorrectly turned off here: https://github.com/alex3165/react-mapbox-gl/blob/6af7ee3f3bb09d901da0da6811c903875553c93c/src/map.tsx#L202-L214

  public componentWillUnmount() {
    const { map } = this.state as State;

    if (map) {
      // Remove all events attached to the map
      map.off();

      // NOTE: We need to defer removing the map to after all children have unmounted
      setTimeout(() => {
        map.remove();
      });
    }
  }

Unless I'm mistake map.off() doesn't remove all event listeners.

Related mapbox-gl-js code: https://github.com/mapbox/mapbox-gl-js/blob/master/src/util/evented.js#L10-L17

    off(type, listener) {
        _removeEventListener(type, listener, this._listeners);
        _removeEventListener(type, listener, this._oneTimeListeners);

        return this;
    }

function _removeEventListener(type, listener, listenerList) {
    if (listenerList && listenerList[type]) {
        const index = listenerList[type].indexOf(listener);
        if (index !== -1) {
            listenerList[type].splice(index, 1);
        }
    }
}

[lamuertepeluda]

@gpbmike you're right, using map.remove() (without delaying) should be enough. See https://github.com/mapbox/mapbox-gl-js/blob/master/src/ui/map.js#L1421. See also #178 .

In general, using timeouts when setting state should be avoided.

[alex3165]

I will close this Issue as it is a duplicated of https://github.com/alex3165/react-mapbox-gl/issues/178

[TLiu2014]

I got the same error. In my case, I used this.setState({ key: value }); in the constructor of a component. It's fixed by replaced with this.state = { key: value };, though this style seems not recommended by React.