alex3165 / react-mapbox-gl

A React binding of mapbox-gl-js
http://alex3165.github.io/react-mapbox-gl/
MIT License
1.93k stars 536 forks source link

changing radius prop of Cluster component doesn't change the view #722

Open qlerebours opened 5 years ago

qlerebours commented 5 years ago

When changing the radius of a Cluster component, the view doesn't change.

For example:

const { clusteringRadius } = this.state;
console.log(clusteringRadius);
return (
    <Cluster ClusterMarkerFactory={this.renderTripCluster} radius={clusteringRadius}>
        ...
    </Cluster>
)

If a button changes clusteringRadius from 0 to 200, it won't change anything, even if my markers are close to each other. Ofc it will log the new clusteringRadius value, but the map doesn't change.

I checked the code behind, the logic is located in componentWillReceiveProps of Cluster.tsx. A call is done to this.childrenChange:

private childrenChange = (
    newChildren: Array<React.ReactElement<MarkerProps>>
  ) => {
    const { superC } = this.state;
    this.featureClusterMap = new WeakMap<
      GeoJSON.Feature,
      React.ReactElement<MarkerProps>
    >();
    const features = this.childrenToFeatures(newChildren);
    superC.load(features);
  };

As you can see, the superC is taken from the state and not redeclared. I think it should check if props have changed and redeclare the superC with:

supercluster({
      radius: nextProps.radius,
      maxZoom: nextProps.maxZoom,
      minZoom: nextProps.minZoom,
      extent: nextProps..extent,
      nodeSize: nextProps.nodeSize,
      log: nextProps.log
    })

Do you think it will cause performance issues ? The aim of this feature would be to be able to let users choose their own clustering radius on a map, with something like a slider.

For the moment, i'm using a crappy workaround like:

this.setState({ clusteringRadius: newVal, hideCluster: true }, () => this.setState({ hideCluster: false })); It gonna unmount Cluster and remount it...

mklopets commented 5 years ago

I think it's reasonable to expect changing the cluster radius to update the actual clustering – valid bug.

materializing-data commented 5 years ago

same problem - any idea if this will be fixed?

qlerebours commented 4 years ago

There's a crappy workaround that is to unmount and remount the component:

  UNSAFE_componentWillReceiveProps(nextProps, nextContext) {
    const { clusteringRadius } = this.props;
    if (clusteringRadius !== nextProps.clusteringRadius) {
      this.setState({ isLoading: true }, () => {
        this.setState({ isLoading: false });
      });
    }
  }

in render:

render() {
  <Map>
    isLoading ? (<Loader />) : (<Cluster />)
  <Map/>
}

Very crappy, but this is working while waiting for a fix