perrycate / tournamap

Website to visualize Smash Ultimate tournaments near you
https://tournamap.gg
GNU General Public License v3.0
9 stars 6 forks source link

Implement list of currently visible tournaments #46

Closed TonCherAmi closed 2 years ago

TonCherAmi commented 2 years ago

You were right about debouncing being the reason behind the list of tournaments becoming inaccurate, I think I forgot to test it after adding it. I've changed the approach to debounce the state itself instead of the function that sets that state and now everything seems to work okay.

perrycate commented 2 years ago

I'm still confused by why we need to pass watch and unwatch around via a context. It seems like a lot of other stuff becomes necessary as a result. Before merging this, can you help me understand why? Is this really the "react way" of doing this?

TonCherAmi commented 2 years ago

We do not need to pass them via a context. You could also pass them through props, though I personally think that would be less clean in this case. I'm not sure I understand what you mean by "a lot of other stuff becomes necessary as a result". Watch/unwatch would still need to be memoized. You would still need to wrap openPopup in useCallback. The dependency array of useEffect inside TournamentMarker would not change at all. The only real difference would be that instead of

const TournamentMarker = memo(({ tournament }) => {
    const markerRef = useRef(null);

    const { watch, unwatch } = useContext(VisibleTournamentsTrackingContext);

    ...
}

you would have

const TournamentMarker = memo(({ tournament, watch, unwatch }) => {
    const markerRef = useRef(null);

    ...
}

and that instead of a provider component you would have a custom hook with pretty much the same code inside.

perrycate commented 2 years ago

Per conversation on stream, I'm merging this now and will attempt to refactor it according to what I think might be possible later. In doing so I fully expect to eventually realize the good reasons for why it's structured as is, but I don't see a reason to delay merging at this point. Thanks for the patience!