Overhaul Venues components by taneliang · Pull Request #3038 · nusmodifications/nusmods

taneliang · 2020-12-08T13:21:26Z

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 VenueList and even the whole VenuesContainer. More details are in the commit messages.

SearchBox changes 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:

Replace Enzyme with React Testing Library #3033: VenuesContainer and SearchBox tests were completely rewritten to use React Testing Library. The tests were rewritten from scratch, so they may need some scrutiny in the PR review.
Replace makeResponsive with useMediaQuery #3032: makeResponsive in VenuesContainer has been replaced by a useMediaQuery call in another component.

Completes #3009 by removing the last use of withRouter.

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 · 2020-12-08T13:25:28Z

Codecov Report

Merging #3038 (381ced3) into master (8e7d591) will increase coverage by 1.61%.
The diff coverage is 81.87%.

@@            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

Impacted Files	Coverage Δ
...ebsite/src/views/components/SearchkitSearchBox.tsx	`0.00% <ø> (ø)`
website/src/views/venues/VenueLocation/index.tsx	`100.00% <ø> (ø)`
...e/src/views/venues/VenueLocation/VenueLocation.tsx	`7.14% <8.33%> (+7.14%)`	⬆️
...e/src/views/venues/VenueLocation/FeedbackModal.tsx	`6.25% <9.09%> (+6.25%)`	⬆️
...rc/views/venues/VenueLocation/AddLocationModal.tsx	`50.00% <50.00%> (ø)`
website/src/views/venues/VenuesContainer.tsx	`77.92% <90.38%> (+5.50%)`	⬆️
website/src/views/venues/VenueDetailsPane.tsx	`92.85% <92.85%> (ø)`
website/src/views/components/SearchBox.tsx	`100.00% <100.00%> (+56.60%)`	⬆️
website/src/views/venues/AvailabilitySearch.tsx	`100.00% <100.00%> (+63.15%)`	⬆️
website/src/views/venues/VenueDetails.tsx	`90.00% <100.00%> (+80.90%)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e7d591...381ced3. Read the comment docs.

nusmods-deploy-bot · 2020-12-08T13:27:08Z

Deployment preview for 381ced32 ready at http://5fddcd3a88a7e5e67dc19b6c--nusmods-deploy-preview.netlify.app

taneliang · 2020-12-08T14:53:51Z

website/src/views/components/SearchBox.tsx

+  // 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);


Let me know if there are objections to this 😆

Usually this is called dirty

taneliang · 2020-12-08T14:57:09Z

website/src/views/venues/VenueDetailsPane.tsx

+  venues: VenueDetailList;
+};
+
+const VenueDetailsPaneComponent: FC<Props> = ({


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.

taneliang · 2020-12-08T14:59:58Z

website/src/views/venues/VenueLocation/AddLocationModal.tsx

+  onRequestClose: () => void;
+};
+
+const AddLocationModal: FC<Props> = ({ venue, isOpen, onRequestClose }) => (


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.

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

taneliang · 2020-12-08T15:01:52Z

website/src/views/venues/VenuesContainer.tsx

+    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


Done in #3040

taneliang · 2020-12-08T15:03:04Z

website/src/views/venues/VenuesContainer.tsx

    return null;
  },
-  render(loaded, props) {
-    return <RoutedVenuesContainer venues={sortVenues(loaded.venues.data)} {...props} />;


This caused so many unnecessary renders 😅

Surprising - is this an issue with React Loadable or are we just not using it right?

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.

ZhangYiJiang · 2020-12-13T01:53:25Z

website/src/views/components/SearchBox.tsx

-      this.setState({ hasChanges: true });
-      if (this.props.useInstantSearch) this.debouncedSearch();
+  const debouncedSearch = useMemo(() => {
+    function search() {


It seems this function can just be inlined

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?

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

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.

It might actually be clearer if we use useCallback

Oh 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.

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.

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).

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 🤷

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

ZhangYiJiang · 2020-12-13T01:54:25Z

website/src/views/components/SearchBox.tsx

+          <Search className={classnames(styles.leftAccessory, styles.searchIcon)} />
+        )}
+        {value && (
+          <X className={styles.removeInput} onClick={clearInput} pointerEvents="bounding-box" />


This should have an aria-label and role - this will make the tests easier to read and improve accessibility

ZhangYiJiang · 2020-12-13T01:56:56Z

website/src/views/venues/AvailabilitySearch.tsx

+    return [
+      (event) => onUpdateInner(event, 'day'),
+      (event) => onUpdateInner(event, 'time'),
+      (event) => onUpdateInner(event, 'duration'),
+    ] as ChangeEventHandler<HTMLSelectElement>[];


Casting isn't safe. Either do the following or use a generic on useMemo to correctly type the returned value

Suggested change

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;

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

this onChange is 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.

website/src/views/venues/VenueDetailsPane.tsx

ZhangYiJiang · 2020-12-13T02:12:36Z

website/src/views/venues/VenueLocation/AddLocationModal.tsx

+  onRequestClose: () => void;
+};
+
+const AddLocationModal: FC<Props> = ({ venue, isOpen, onRequestClose }) => (


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

ZhangYiJiang · 2020-12-13T02:50:04Z

website/src/views/venues/VenuesContainer.test.tsx

+  const rendered = renderWithRouterMatch(
+    <>
+      <VenuesContainerComponent venues={venues} />
+      <div id={modalElementId} />


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();

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

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);

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

ZhangYiJiang · 2020-12-13T02:55:18Z

website/src/views/components/SearchBox.test.tsx

+    expect(box).not.toHaveFocus();
+    expect(mockOnBlur).toHaveBeenCalledTimes(1);
+    expect(mockOnChange).not.toHaveBeenCalled();
+    expect(mockOnSearch).not.toHaveBeenCalled();


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?

Maybe we could do something like await expect(waitFor(() => expect(mockOnSearch).toHaveBeenCalled())).rejects.toThrowError()? Hahaha

We could mock debounce instead to immediately invoke the function.

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?

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?

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 😆

ZhangYiJiang · 2020-12-13T02:57:17Z

website/src/views/venues/VenuesContainer.test.tsx

+      expect(backButton).toBeInTheDocument();
+
+      userEvent.click(backButton);
+      expect(backButton).not.toBeInTheDocument();


Maybe check URL too (or instead? Checking the button is gone is a bit weird)

website/src/views/venues/VenuesContainer.test.tsx

ZhangYiJiang · 2020-12-13T03:00:22Z

website/src/views/venues/VenuesContainer.tsx

    return null;
  },
-  render(loaded, props) {
-    return <RoutedVenuesContainer venues={sortVenues(loaded.venues.data)} {...props} />;


Surprising - is this an issue with React Loadable or are we just not using it right?

ZhangYiJiang · 2020-12-13T03:03:52Z

website/src/views/components/SearchBox.tsx

+  const clearInput = useCallback(() => {
+    onChange('');
+    hasChanges.current = true;
+    debouncedSearch();


This should flush I think, feels like an oversight not having it

So turns out if we add a flush we break SearchkitSearchBox because Searchkit's search is triggered before SearchkitSearchBox's state input is updated.

Ooof. Okay worth a comment

taneliang · 2020-12-13T04:31:23Z

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.

…tainer

li-kai

I'll give the go ahead. I don't see any major issues with this code change. ☃️

li-kai · 2020-12-19T07:45:30Z

website/src/views/components/SearchBox.test.tsx

+    expect(box).not.toHaveFocus();
+    expect(mockOnBlur).toHaveBeenCalledTimes(1);
+    expect(mockOnChange).not.toHaveBeenCalled();
+    expect(mockOnSearch).not.toHaveBeenCalled();


We could mock debounce instead to immediately invoke the function.

li-kai · 2020-12-19T07:54:06Z

website/src/views/venues/VenueDetailsPane.tsx

+    const [previous] = matchedVenues[venueIndex - 1] ?? [];
+    const [next] = matchedVenues[venueIndex + 1] ?? [];
+    return { venue, availability, next, previous };


Suggested change

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] };

Oh you'll need to pull out the 0th value, so it'll look more like

Suggested change

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?

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:

Suggested change

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.

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

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?

taneliang · 2020-12-19T09:51:14Z

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.

taneliang added 20 commits December 7, 2020 12:06

Inline single-use pageHead

1b81be4

Memoize expensive VenueList variable computation

f573658

Though, this doesn't currently have any effect as `VenuesContainer` passes in a new `venues` prop on every render.

window.location -> useLocation in VenueList

564e2e1

To support non-DOM environments (e.g. tests that use MemoryRouter)

Improve AvailabilitySearch memoization

689b03a

Roughly functionalize VenuesContainer

0284997

Cache sorted venues in LoadableVenuesContainer to prevent memo invali…

4d4c5a3

…ation on every render

Fix updateURL (and inline it into a useEffect)

100433e

Functionalize FeedbackModal

26790bc

Functionalize VenueLocation

c732513

Inline + extract AddLocationModal from VenueLocation renderFeedbackMe…

806c77e

…nu to improve clarity

Reduce VenueList renders as they are very expensive

ee6266a

Extract SecondaryPane into VenueDetailsPane.tsx

9c14cef

Rewrite VenuesContainer tests

fe07f0b

Remove unstable_batchedUpdates as React already batches updates withi…

aa79d23

…n React event handlers

SearchBox: Remove useInstantSearch as it's always true

2238100

SearchBox: Rewrite test for comprehensiveness

8e2b0be

Functionalize SearchBox

bce257b

SearchBox: Replace hasChanges state with mutable ref

fc989e7

SearchBox: Inline search callback into debouncedSearch

9cb8c72

taneliang added performance better engineering labels Dec 8, 2020

This was referenced Dec 8, 2020

Adopt React concurrent mode #3039

Closed

Optimize Venues search using React Concurrent Mode APIs #3040

Open

taneliang commented Dec 8, 2020

View reviewed changes

taneliang marked this pull request as ready for review December 8, 2020 15:11

taneliang requested a review from a team December 8, 2020 15:11

ZhangYiJiang reviewed Dec 13, 2020

View reviewed changes

taneliang mentioned this pull request Dec 17, 2020

Replace makeResponsive with useMediaQuery #3032

Open

taneliang added 10 commits December 18, 2020 14:33

Merge branch 'master' into eliang/overhaul-venues-components

76b03a3

Fix review comment: hasChanges -> isDirty

4d234e8

Fix review comment: inline search

d5b270e

Fix review comment: improve clear button accessibility

73f8c96

Fix review comment: remove unsafe casting

768fa79

Remove unnecessary type cast

46ab5bd

Fix review comment: remove handleInput instanceof check

4427239

Fix review comment: remove special modal element in favor of view.con…

98b62af

…tainer

Add SearchBox debouncedSearch FIXME note

db261b5

Split VenuesContainer modal test into 2

9d72b35

li-kai approved these changes Dec 19, 2020

View reviewed changes

taneliang added 2 commits December 19, 2020 17:30

Merge branch 'master' into eliang/overhaul-venues-components

701099a

Add comment explaining why we don't flush in clearInput

381ced3

Conversation

taneliang commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Details

Uh oh!

codecov bot commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nusmods-deploy-bot bot commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taneliang Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

taneliang commented Dec 8, 2020 •

edited

Loading

codecov bot commented Dec 8, 2020 •

edited

Loading

nusmods-deploy-bot bot commented Dec 8, 2020 •

edited

Loading

taneliang Dec 20, 2020 •

edited

Loading