Overhaul Venues components #3038
Conversation
Cost of `memo` is probably higher than perf savings for `VenueDetails`. All expensive variables in `VenueDetails` are memoized now, and `Timetable` is a PureComponent, so rendering `VenueDetails` should be cheap. Also, `VenuesContainer` (the only component that uses `VenueDetails`) doesn't take advantage of memoization anyway as it passes a new `highlightPeriod` object on every render.
Though, this doesn't currently have any effect as `VenuesContainer` passes in a new `venues` prop on every render.
To support non-DOM environments (e.g. tests that use MemoryRouter)
…ation on every render
…nu to improve clarity
…n React event handlers
Codecov Report
@@ Coverage Diff @@
## master #3038 +/- ##
==========================================
+ Coverage 55.58% 57.20% +1.61%
==========================================
Files 255 257 +2
Lines 5311 5283 -28
Branches 1218 1205 -13
==========================================
+ Hits 2952 3022 +70
+ Misses 2359 2261 -98
Continue to review full report at Codecov.
|
|
Deployment preview for |
| // This is a ref instead of state as it is only used within event handlers and | ||
| // we don't want to trigger unnecessary renders when it's changed. | ||
| const hasChanges = useRef(false); |
There was a problem hiding this comment.
Let me know if there are objections to this 😆
There was a problem hiding this comment.
Usually this is called dirty
| venues: VenueDetailList; | ||
| }; | ||
|
|
||
| const VenueDetailsPaneComponent: FC<Props> = ({ |
There was a problem hiding this comment.
Not sure if this file name (VenueDetailsPane) makes sense, as there's a VenueDetails component as well. I'm calling it a pane as it's basically the right pane of the Venues page's split view. Suggestions welcome.
Btw, there's no LeftPane or VenueListPane because the left pane depends very heavily on VenuesContainer state, which is also used by VenueDetailsPane. We could consider using a context but I don't think it's worth it.
| onRequestClose: () => void; | ||
| }; | ||
|
|
||
| const AddLocationModal: FC<Props> = ({ venue, isOpen, onRequestClose }) => ( |
There was a problem hiding this comment.
Moved this into its own component (and file) as it seemed unclean that FeedbackModal was its own component and file but the add location modal was inline.
There was a problem hiding this comment.
Mostly because it's just a thin wrapper around ImproveVenueForm so the code is very trivial, but making it SLAP is not a bad idea
| setSearchQuery, | ||
| ] = useState<string>(() => qs.parse(location.search).q || ''); | ||
| /** Actual string to search with; deferred update */ | ||
| const deferredSearchQuery = searchQuery; // TODO: Redundant now. Use React.useDeferredValue after we adopt concurrent mode |
| @@ -384,8 +248,8 @@ const AsyncVenuesContainer = Loadable.Map<Subtract<Props, LoadedProps>, { venues | |||
|
|
|||
| return null; | |||
| }, | |||
| render(loaded, props) { | |||
| return <RoutedVenuesContainer venues={sortVenues(loaded.venues.data)} {...props} />; | |||
There was a problem hiding this comment.
This caused so many unnecessary renders 😅
There was a problem hiding this comment.
Surprising - is this an issue with React Loadable or are we just not using it right?
There was a problem hiding this comment.
I think we were just not using it right. We were passing a new venues prop to RoutedVenuesContainer on every render, so any memoized value/component that used venues would always be recomputed.
| this.setState({ hasChanges: true }); | ||
| if (this.props.useInstantSearch) this.debouncedSearch(); | ||
| const debouncedSearch = useMemo(() => { | ||
| function search() { |
There was a problem hiding this comment.
It seems this function can just be inlined
There was a problem hiding this comment.
I did try that, but I felt that the resulting debounce(() => {...}, throttle, { leading: false }) was harder to understand, but maybe it's pretty obvious since this var is called debouncedSearch. What do you think?
There was a problem hiding this comment.
Yeah, I think it's clear enough, esp. since it's obviously a wrapper around onSearch. It might actually be clearer if we use useCallback -
const debounceedSearch = useCallback(debounce(() => {
hasChanges.current = false;
onSearch();
}, throttle, { leading: false }, [onSearch, throttle]);I'm actually not sure if the debounce behavior will work correctly if throttle or onSearch changes and cause a recomputation lol, but oh well
There was a problem hiding this comment.
It might actually be clearer if we use
useCallback
Oh yeah, good catch.
I'm actually not sure if the debounce behavior will work correctly if throttle or onSearch changes and cause a recomputation
Yeah this may be a bug. I'll add a FIXME, but currently the throttle and onSearch props are always passed stable values.
There was a problem hiding this comment.
It might actually be clearer if we use
useCallbackOh yeah, good catch.
Actually no, this won't work well as debounce returns something that's more than a function and some lint rules fail. I think useMemo is more appropriate.
There was a problem hiding this comment.
Hmmm. So the lint rule is not about types (notice TS is perfectly happy here), but rather because the exhaustive-deps rule depends on the parameter being an inline function so that it can statically analyze its content for deps. I guess we have to think about whether the clarity of useCallback is worth trading off exhaustive-deps no longer working here. No strong opinions either way.
There was a problem hiding this comment.
Yep that is true. I guess my bigger objection is that we're trying to memoize something that's more than a function. Since useCallback seems to be intended to memoize functions (from the docs: useCallback(**fn**, deps)), I think it may be a little unsafe to assume that useCallback will return the exact callback that's passed to it (even though it currently does).
There was a problem hiding this comment.
So this is getting philosophical, but functions are objects in JS. Functions have both native properties, and can have additional properties attached to them (React in fact uses this itself with displayName and defaultProps on functional components). Any function that wants to correctly proxy function should take this into account.
On a more practical level, the reason useCallback exists at all, other than being sugar, is the assumption that declaring a function is cheap, so it is okay to write useCallback(() => { /* some long function declaration */ }) since just declaring the function is fast. IMO _.debounce is trivial enough that this is fine. But having the linter work is probably more valuable. So 🤷
There was a problem hiding this comment.
Good points. I was concerned that useCallback would potentially do something weird in the future like wrapping the callback in an IIFE, but your argument makes sense.
I guess we'll just leave this as is so that we don't have to disable the lint rule
| <Search className={classnames(styles.leftAccessory, styles.searchIcon)} /> | ||
| )} | ||
| {value && ( | ||
| <X className={styles.removeInput} onClick={clearInput} pointerEvents="bounding-box" /> |
There was a problem hiding this comment.
This should have an aria-label and role - this will make the tests easier to read and improve accessibility
| return [ | ||
| (event) => onUpdateInner(event, 'day'), | ||
| (event) => onUpdateInner(event, 'time'), | ||
| (event) => onUpdateInner(event, 'duration'), | ||
| ] as ChangeEventHandler<HTMLSelectElement>[]; |
There was a problem hiding this comment.
Casting isn't safe. Either do the following or use a generic on useMemo to correctly type the returned value
| return [ | |
| (event) => onUpdateInner(event, 'day'), | |
| (event) => onUpdateInner(event, 'time'), | |
| (event) => onUpdateInner(event, 'duration'), | |
| ] as ChangeEventHandler<HTMLSelectElement>[]; | |
| const handlers: ChangeEventHandler<HTMLSelectElement>[] = [ | |
| (event) => onUpdateInner(event, 'day'), | |
| (event) => onUpdateInner(event, 'time'), | |
| (event) => onUpdateInner(event, 'duration'), | |
| ]; | |
| return handlers; |
There was a problem hiding this comment.
The useMemo is probably not very useful anyway - the only of the props that should change often is searchOptions, which is also a dep here. onUpdate can be changed to the Dispatch form to remove that as a dep but this onChange is passed into a native select anyway so I don't think the performance difference is huge
There was a problem hiding this comment.
this
onChangeis passed into a native select anyway so I don't think the performance difference is huge
True. And yeah it's definitely not very clean to have 2 props change together in 2 different components. I think it's fine to leave it as is though (after fixing that typecast). We can improve this some other time.
| onRequestClose: () => void; | ||
| }; | ||
|
|
||
| const AddLocationModal: FC<Props> = ({ venue, isOpen, onRequestClose }) => ( |
There was a problem hiding this comment.
Mostly because it's just a thin wrapper around ImproveVenueForm so the code is very trivial, but making it SLAP is not a bad idea
| const rendered = renderWithRouterMatch( | ||
| <> | ||
| <VenuesContainerComponent venues={venues} /> | ||
| <div id={modalElementId} /> |
There was a problem hiding this comment.
I'm not sure if this is correct? Why do we need this?
If we do want this the following might be a bit more ergonomic?
function setupReactModal() {
return {
rootElement: <div id="test-react-modal-root" />,
init: () => ReactModal.setAppElement('#test-react-modal-root'),
}
}
// ...
const testReactModal = setupReactModal();
render(
<>
{...}
{testReactModal.rootElement}
</>
);
testReactModal.init();There was a problem hiding this comment.
Why do we need this?
ReactModal just needs an element to render into, otherwise it'll throw an error (iirc). This was the best way I could come up with to fix that. Do you have any other ideas? Your suggestion definitely looks cleaner, so if we can't think of any other way we can just do that
There was a problem hiding this comment.
Hmmm well I'm mainly worried because the API wants the root element, but I don't know how that works with React Testing Library (their docs don't mention anything about it). If we just need an element that's in the DOM you could just do this in the framework setup code
cost modalRoot = document.createElement('div');
document.body.appendChild(modalRoot);
ReactModal.setRootElement(modalRoot);There was a problem hiding this comment.
the API wants the root element
Ah right. In that case maybe we can just use ReactModal.setAppElement(rendered.view.container as HTMLElement);, then we don't need any setup code
| expect(box).not.toHaveFocus(); | ||
| expect(mockOnBlur).toHaveBeenCalledTimes(1); | ||
| expect(mockOnChange).not.toHaveBeenCalled(); | ||
| expect(mockOnSearch).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
onSearch is debounced, so testing by counting calls might not be accurate. Not sure if there is a better way though, maybe add some mock timer waits to force for debounce to clear?
There was a problem hiding this comment.
Maybe we could do something like await expect(waitFor(() => expect(mockOnSearch).toHaveBeenCalled())).rejects.toThrowError()? Hahaha
There was a problem hiding this comment.
We could mock debounce instead to immediately invoke the function.
There was a problem hiding this comment.
Not a bad idea, but this will couple our test to an implementation detail of SearchBox :(
Tbh I think this debouncing/throttling logic should be moved into SearchkitSearchBox instead since it's now only relevant for module search. I think we should just file an issue for this and defer this discussion to another time?
There was a problem hiding this comment.
I don't buy into the implementation detail test article as much. It is useful to keep in mind, but patterns like the page object pattern (why I asked if we should extract some of the queries into constants or functions) are more useful IMO to serve the goal of improving test robustness and reducing breakage when implementation changes.
Here specifically we want to test implementation details, because testing exactly what the user sees would be kind of ridiculous - the goal of the debounce is to improve rendering performance, and to test that in a concrete manner would probably involve, what, rendering this with some arbitrary CPU throttling to prove it is fast enough?
There was a problem hiding this comment.
the goal of the debounce is to improve rendering performance
I don't think this is true anymore. The venues page previously used this for performance, but now that the venues search uses useDeferredValue for perf (at least after #3040), this is only used to throttle our module search so that we don't make too many requests. Venues search no longer uses onSearch.
That's why this throttling/debouncing stuff should be moved into SearchkitSearchBox imo, and we can lazily sidestep this discussion entirely since SearchkitSearchBox doesn't have tests 😆
| expect(backButton).toBeInTheDocument(); | ||
|
|
||
| userEvent.click(backButton); | ||
| expect(backButton).not.toBeInTheDocument(); |
There was a problem hiding this comment.
Maybe check URL too (or instead? Checking the button is gone is a bit weird)
| @@ -384,8 +248,8 @@ const AsyncVenuesContainer = Loadable.Map<Subtract<Props, LoadedProps>, { venues | |||
|
|
|||
| return null; | |||
| }, | |||
| render(loaded, props) { | |||
| return <RoutedVenuesContainer venues={sortVenues(loaded.venues.data)} {...props} />; | |||
There was a problem hiding this comment.
Surprising - is this an issue with React Loadable or are we just not using it right?
| const clearInput = useCallback(() => { | ||
| onChange(''); | ||
| hasChanges.current = true; | ||
| debouncedSearch(); |
There was a problem hiding this comment.
This should flush I think, feels like an oversight not having it
There was a problem hiding this comment.
So turns out if we add a flush we break SearchkitSearchBox because Searchkit's search is triggered before SearchkitSearchBox's state input is updated.
There was a problem hiding this comment.
Ooof. Okay worth a comment
|
Thanks for the detailed review @ZhangYiJiang! I'll likely address the comments much later in the week as I have in-camp training until Thursday. |
| expect(box).not.toHaveFocus(); | ||
| expect(mockOnBlur).toHaveBeenCalledTimes(1); | ||
| expect(mockOnChange).not.toHaveBeenCalled(); | ||
| expect(mockOnSearch).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
We could mock debounce instead to immediately invoke the function.
| const [previous] = matchedVenues[venueIndex - 1] ?? []; | ||
| const [next] = matchedVenues[venueIndex + 1] ?? []; | ||
| return { venue, availability, next, previous }; |
There was a problem hiding this comment.
| const [previous] = matchedVenues[venueIndex - 1] ?? []; | |
| const [next] = matchedVenues[venueIndex + 1] ?? []; | |
| return { venue, availability, next, previous }; | |
| return { venue, availability, next: matchedVenues[venueIndex - 1], previous: matchedVenues[venueIndex + 1] }; |
There was a problem hiding this comment.
Oh you'll need to pull out the 0th value, so it'll look more like
| const [previous] = matchedVenues[venueIndex - 1] ?? []; | |
| const [next] = matchedVenues[venueIndex + 1] ?? []; | |
| return { venue, availability, next, previous }; | |
| return { | |
| venue, | |
| availability, | |
| next: matchedVenues[venueIndex - 1]?.[0], | |
| previous: matchedVenues[venueIndex + 1]?.[0], | |
| }; |
which yeah I think is pretty clear as well. Let's use this?
There was a problem hiding this comment.
Actually I think the original code is clearest as it shows a clearer relationship between previous, next, and the const [venue, availability] = matchedVenues[venueIndex]; line. I think we should leave it as that.
Alternatively:
| const [previous] = matchedVenues[venueIndex - 1] ?? []; | |
| const [next] = matchedVenues[venueIndex + 1] ?? []; | |
| return { venue, availability, next, previous }; | |
| return { | |
| venue: matchedVenues[venueIndex][0], | |
| availability: matchedVenues[venueIndex][1], | |
| next: matchedVenues[venueIndex - 1]?.[0], | |
| previous: matchedVenues[venueIndex + 1]?.[0], | |
| }; |
which I think is less clear.
There was a problem hiding this comment.
Nah I think the tuple destructure version is still the clearest. Might be clearer if you explicitly name the default tuple (since it might be confused with the parent array) but that's minor
There was a problem hiding this comment.
Hmm personally I think naming the default tuple will be less clear since readers will need to look up what the default tuple actually is
But okay anyway I think we're all in agreement that we should just leave this unchanged?
|
All done! I'll wait for #3040 to be done before merging this stack, so that master doesn't end up with a slow venues page. |
Context
This is a rather large PR that makes various changes to the components on the Venues page.
Details
The biggest (and probably only?) user-facing change is that search box optimizations have been removed. This will be restored in #3040 with optimizations that use concurrent mode. As a result, we'll want to land this stack of 3 PRs together to prevent performance regressions in prod.
This PR does optimize a few components. Specifically, it reduces unnecessary renders of expensive components like
VenueListand even the wholeVenuesContainer. More details are in the commit messages.SearchBoxchanges aren't strictly related to the rest of the changes, but they were necessary to allow #3040 to experiment with concurrent mode APIs in the search box. I didn't use any of those experiments though.Similar to #3007 and #3009, this PR also works towards:
VenuesContainerandSearchBoxtests were completely rewritten to use React Testing Library. The tests were rewritten from scratch, so they may need some scrutiny in the PR review.makeResponsivewithuseMediaQuery#3032:makeResponsiveinVenuesContainerhas been replaced by auseMediaQuerycall in another component.Completes #3009 by removing the last use of
withRouter.