react-mapbox-gl icon indicating copy to clipboard operation
react-mapbox-gl copied to clipboard

Fix memoized rebind

Open akre54 opened this issue 4 years ago • 0 comments

This is a strawman fix for #927. Currently react-mapbox-gl binds DOM events the first time a prop function is passed in, which means that for memoized functions (for instance, with useMemo / React functional components) the handler may be a stale closure and have out-of-date data.

The issue is that the updateEvents function in map-events.tsx only checks for the presence of the function but not referential equality, and doesn't dereference the latest value off of this.props.

As I see it, there are 3 approaches we could take to fix this in library code:

  1. Unbind stale functions and re-bind new functions when passed in (i.e. change the toListenOff expression to check against prevProps -- see the first commit here).
    1. Dereference the latest value of the callback in this.props (the approach I took here in the second commit). The issue with this approach is that we need access to the this object in updateEvents, because the props changes between renders, so this is a bit uglier than I'd like.
  2. Use a Ref, always set to the latest passed-in prop value. This seems like a duplication of 2 with extra steps.

The last approach would be to migrate map.tsx to be a functional component, but that might be a bigger ask.

Any suggestions here? Thanks!

akre54 avatar Apr 28 '21 19:04 akre54