refactor(AdvancedMarker): improve stability of position and event handlers #866
Conversation
| } = props; | ||
| const positionLng = positionProp?.lng; | ||
| const positionLat = positionProp?.lat; | ||
| const position = useMemo( |
There was a problem hiding this comment.
The reason why I added this was so that devs can still send inline positions to AdvancedMarker, e.g:
return <AdvancedMarker position={{ lat:1, lng:1}}>
And we make sure to not set the marker to this position again (which can trigger potentially heavy calculations on google maps API side. At least to my knowledge they don't do a deep comparison to check if the previously assigned position is deeply different from the new one.
| * @public | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export function useEffectEvent<const T extends (...args: any[]) => void>( |
There was a problem hiding this comment.
This is based on https://github.com/sanity-io/use-effect-event/blob/main/src/useEffectEvent.ts and on a ponyfill that I use at my company.
But for transparency, I essentially copied the one from https://github.com/sanity-io/use-effect-event/blob/main/src/useEffectEvent.ts, which is currently widely used, and removed their extra check that uses some weird React.use thing to make sure devs don't use this useEffectEvent in render. Reasons I didn't add that part:
- It's simpler this way
- Eslint react plugin takes care of identifying if we only use it for useEffect events or if we use it somewhere else.
| * Internally used to bind events to DOM nodes. | ||
| * @internal | ||
| */ | ||
| export function useDomEventListener<T extends (...args: any[]) => void>( |
There was a problem hiding this comment.
For both useDomEventListener and useMapsEventListener, to prevent adding and removing listeners on every render (which was the previous logic), I verify if the callback is defined and I useEffectEvent as the callback. eslint makes sure callbackEvent is not set on useEffect dependencies.
The tricky part of useEffectEvent, both the ponyfill and the "native" implementation, is that the returned function is a new one every time. As such, we should not directly assign callbackEvent to the addEventListener or addListener functions as we risk sending a different function on removeEventListener than the one on addEventListener, which could theoretically cause a memory leak.
I believe this is per design as you can see on https://github.com/facebook/react/blob/b7e2de632b2a160bc09edda1fbb9b8f85a6914e8/packages/react-reconciler/src/ReactFiberHooks.js#L2728
2 participants
Hi there 👋
For a library like this one that relies heavily on correct and stable rendering of useEffect and useMemo dependencies, I suggest we add a useEffectEvent ponyfill, to fill that gap.
This ponyfill is proven to work and the new version of eslint react-hooks plugin, that is already installed, supports verifying and using these events.
I took the freedom to add it in key places like useMapsEventListener and useDomEventListener.