Remove `withRouter` (1/n) #3007

taneliang · 2020-12-01T10:13:28Z

Context

As #2922 requires extensive changes to the whole codebase, I've decided to split it into multiple PRs.

This PR works towards React Router 6 compatibility by removing calls to withRouter. RR6 is necessary for preloading support, which is necessary in #2922. This is incomplete and will be continued in a future PR, but I think it may be best to get some feedback on the current work first.

Implementation

Frontend tests of hoc-wrapped components will likely need to be changed significantly, because:
1. With hooks, we can't e.g. pass in history and Redux state props. We need to use MemoryRouter/Router and Redux's Provider instead.
2. Tests frequently assume that data will be passed in as props, which may no longer be the case once we switch to Relay/Apollo. Tests that are written from the end user's perspective will help us prepare for this.
3. Some tests assume the component will be a class component, and test their methods directly.
Added @testing-library/react so that it's easier to test components in context and from a user's perspective.
makeResponsive is deprecated with a new useMediaQuery hook.
A new global __TEST__ variable is declared, as we sometimes need to distinguish between __DEV__ and __TEST__.

GlobalSearchContainer tests have been rewritten with this new approach. Would appreciate feedback!

For a more modern testing API that: 1. Discourages testing implementation details. 2. Makes it easier to test user-visible behavior.

Prepares GlobalSearchContainer tests for functionalization. The existing tests tested methods on the GlobalSearchContainer class, which were an implementation detail (those methods were meant to be called by the child `GlobalSearch` component). Those methods will be turned into `useCallback` callbacks in a functionalized component not accessible from outside the component, requiring a rewrite of the tests.

Preparing to replace those HOCs with hooks.

…er tests

codecov · 2020-12-01T10:17:07Z

Codecov Report

Merging #3007 (f019ae1) into master (089eb58) will increase coverage by 1.42%.
The diff coverage is 26.90%.

@@            Coverage Diff             @@
##           master    #3007      +/-   ##
==========================================
+ Coverage   46.35%   47.78%   +1.42%     
==========================================
  Files         252      254       +2     
  Lines        5324     5316       -8     
  Branches     1224     1218       -6     
==========================================
+ Hits         2468     2540      +72     
+ Misses       2856     2776      -80

Impacted Files	Coverage Δ
website/src/.eslintrc.js	`0.00% <ø> (ø)`
website/src/actions/venueBank.ts	`33.33% <ø> (ø)`
website/src/bootstrapping/configure-store.ts	`68.75% <ø> (+68.75%)`	⬆️
website/src/bootstrapping/sentry.ts	`0.00% <0.00%> (ø)`
website/src/entry/main.tsx	`0.00% <0.00%> (ø)`
website/src/utils/react.tsx	`72.97% <0.00%> (-5.98%)`	⬇️
website/src/views/AppShell.tsx	`0.00% <0.00%> (ø)`
website/src/views/components/KeyboardShortcuts.tsx	`0.00% <0.00%> (ø)`
...ribute/ContributeContainer/ContributeContainer.tsx	`0.00% <0.00%> (ø)`
website/src/views/modules/ModulePageContent.tsx	`74.41% <ø> (ø)`
... and 23 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 089eb58...f019ae1. Read the comment docs.

nusmods-deploy-bot · 2020-12-01T10:18:23Z

Deployment preview for f019ae14 ready at http://5fc863028753467c6df91dc6--nusmods-deploy-preview.netlify.app

taneliang · 2020-12-02T05:27:55Z

website/src/utils/css.ts

-export function queryMatch(query: QueryObject) {
-  return window.matchMedia(json2mq(query));
-}


Dead code originally used in module search before we moved to Elasticsearch

taneliang · 2020-12-02T05:31:22Z

website/src/views/AppShell.tsx


    // Fetch the module data of the existing modules in the timetable and ensure all
    // lessons are filled
+    const timetables = store.getState().timetables.lessons;


Not using useSelector here, because if we bind timetables outside this hook:

We'll need to pass timetables as a dependency to useCallback, which will cause the callback to change when the user modifies the timetable. This will cause the effect below to be executed, which is not what we want.

If we don't pass timetables as a dependency, the callback may capture a old copy of timetables, which will cause the incorrect modules to be fetched.

website/src/views/components/KeyboardShortcuts.tsx

taneliang · 2020-12-02T05:34:11Z

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

    expect(window.scrollTo).toHaveBeenCalledTimes(1);
    expect(window.scrollTo).toHaveBeenCalledWith(0, 0);
-    history.push('/foo');
+    act(() => history.push('/foo'));


act is needed to flush effects

ZhangYiJiang

Man, this is truly a heroic effort. Props to you for doing this :P

website/src/utils/css.ts

website/src/views/components/KeyboardShortcuts.tsx

ZhangYiJiang · 2020-12-02T05:43:24Z

website/src/views/components/RandomKawaii.tsx

+  const [Kawaii] = useState(() => sample(icons) ?? icons[0]);
+  const [defaultMood] = useState(() => sample(defaultMoods) ?? defaultMoods[0]);


Using state to only initialize once instead of using refs is rather weird lol. It looks like there's an easy way to write a hook for this that does it correctly facebook/react#14490 (comment).

Also array access like this is not really safe anyway (if sample returns undefined then the array is empty, so [0] would also return undefined), so just use !

Using state to only initialize once instead of using refs is rather weird lol.

Hmm, I'm not sure if the additional code overhead is worth it. Also looks like someone else also suggested using useState in that issue 😆. IMO useState is the best solution here as conceptually what we want is literally state that we don't want to change. I don't think it's exactly a ref either as refs tend to imply some sort of imperative/mutable variable.

Also array access like this is not really safe anyway (if sample returns undefined then the array is empty, so [0] would also return undefined), so just use !

Yeah I just wanted to avoid the noise of eslint-disable, but sure that works too. What we really want is nullthrows but probably it's probably not worth adding it for just one callsite.

website/src/views/components/ScrollToTop.tsx

website/src/views/layout/GlobalSearchContainer.test.tsx

ZhangYiJiang · 2020-12-02T06:16:53Z

website/src/views/AppShell.tsx

+/**
+ * Fetch module list on mount.
+ */
+function useFetchModuleListAndTimetableModules(): {


Do we need to get rid of connect? This feels a fair bit more awkward then the old code, though having good typings is nice

What feels awkward? If this hook is weird, maybe we could just inline it into the AppShell component. I just thought it was nicer to isolate the fetching logic into its own hook so that we didn't have so many variables lying around. I don't think we need to get rid of connect, but I don't think keeping it around helps much?

`ScrollToTop` was used in exactly 1 way: scroll to top/hash on mount. The new hook implements that and removes all configuration and props.

website/src/views/layout/GlobalSearchContainer.tsx

website/src/views/hooks/useScrollToTopEffect.ts

ZhangYiJiang · 2020-12-03T00:59:24Z

website/src/views/hooks/useScrollToTopEffect.test.tsx

+jest.mock('utils/react');
+const mockedScrollToHash = scrollToHash as jest.MockedFunction<typeof scrollToHash>;
+
+const Tester: FC = () => {


Maybe consider using the hook testing library https://react-hooks-testing-library.com/usage/basic-hooks

Hmm, I think having an explicit tester component is clearer? I didn't think using that library helps much since this component is only 2 lines long, and we'll still need to provide a Router wrapper (similar to the snippet below from hook testing library docs). The additional indirection seems to provide little value in this case.

const wrapper = ({ children }) => <CounterStepProvider step={2}>{children}</CounterStepProvider> const { result } = renderHook(() => useCounter(), { wrapper })

website/src/views/components/RandomKawaii.tsx

ZhangYiJiang · 2020-12-03T02:48:31Z

website/src/views/layout/GlobalSearchContainer.test.tsx

+      draft.moduleBank.moduleList = (storeOverrides.moduleBank?.moduleList ??
+        relevantStoreContents.moduleBank.moduleList) as typeof draft.moduleBank.moduleList;


Ah. I guess it is possible to fix the typing of MODULES, but it's a bit awkward (we need to add this util type microsoft/TypeScript#24509). Cast is probably fine too, since this is just a test

ZhangYiJiang · 2020-12-03T02:50:07Z

website/src/views/layout/GlobalSearchContainer.test.tsx

+    let onMediaChangeCallback: () => void;
+    const addEventListener = (_type: string, listener: (...args: unknown[]) => void) => {
+      onMediaChangeCallback = listener;
+    };


Might be simpler to just mock the entire hook, but this does also add some coverage to the hook itself, so 🤷

We probably should add tests for the hook itself and simplify the tests here. Was intending to do it but forgot. I'll file an issue to fix another time

Created issue #3011.

taneliang · 2020-12-03T03:55:22Z

Thanks for the quick reviews @ZhangYiJiang 🏃‍♂️

taneliang added 15 commits November 30, 2020 14:37

Functionalize RandomKawaii

d9f468b

Functionalize KeyboardShortcuts

44cd5ce

Functionalize ScrollToTop

e125590

Functionalize AppShell

93bcb64

Functionalize LessonTimetable

6573396

Functionalize ModRegNotification

f5ccc16

Add React Testing Library

4559d71

For a more modern testing API that: 1. Discourages testing implementation details. 2. Makes it easier to test user-visible behavior.

Introduce __TEST__ global var

34c8bff

Test GlobalSearchContainer wrapped in withRouter and connect HOCs

8846526

Preparing to replace those HOCs with hooks.

Functionalize GlobalSearchContainer

87ec3ac

Define new useMediaQuery hook to replace makeResponsive HOC

08d7e1f

Make mockDom mock more robust; mock matchMedia in GlobalSearchContain…

2008c60

…er tests

Replace makeResponsive -> useMediaQuery in GlobalSearchContainer

aea48bb

Fix LessonTimetable type error

19e9e08

taneliang requested a review from a team December 1, 2020 10:13

taneliang added 4 commits December 2, 2020 12:35

Add @testing-library/* to Renovate test group

b31bc83

Fix nits in css.ts

10e4f49

Migrate ScrollToTop from enzyme -> RTL

d925401

Fix nits

6b84a2c

taneliang commented Dec 2, 2020

View reviewed changes

ZhangYiJiang reviewed Dec 2, 2020

View reviewed changes

taneliang added 5 commits December 2, 2020 15:39

Split GlobalSearchContainer show many results test

b675711

Fix review comments

02e91a6

Define new, simpler useScrollToTopEffect hook to replace ScrollToTop

fb67e1f

`ScrollToTop` was used in exactly 1 way: scroll to top/hash on mount. The new hook implements that and removes all configuration and props.

Nit: clarify useScrollToTopEffect useEffect comment

2e4f29a

Nit: further clarify useScrollToTopEffect comments

8b0cf98

taneliang mentioned this pull request Dec 2, 2020

Remove withRouter (2/2) #3009

Merged

taneliang mentioned this pull request Dec 3, 2020

GlobalSearch options have poor readability in screen readers #3010

Closed

ZhangYiJiang approved these changes Dec 3, 2020

View reviewed changes

taneliang mentioned this pull request Dec 3, 2020

Add tests for useMediaQuery hook #3011

Closed

Fix review comments

f019ae1

taneliang added the automerge label Dec 3, 2020

github-actions bot merged commit f39465d into master Dec 3, 2020

github-actions bot deleted the eliang/more-fc branch December 3, 2020 04:01

chrisgzf mentioned this pull request Jan 16, 2021

Fix keyboard shortcuts snackbar text showing stale values #3165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `withRouter` (1/n) #3007

Remove `withRouter` (1/n) #3007

taneliang commented Dec 1, 2020

codecov bot commented Dec 1, 2020 •

edited

Loading

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

edited

Loading

taneliang Dec 2, 2020

taneliang Dec 2, 2020

taneliang Dec 2, 2020

ZhangYiJiang left a comment

ZhangYiJiang Dec 2, 2020

taneliang Dec 2, 2020 •

edited

Loading

ZhangYiJiang Dec 2, 2020

taneliang Dec 2, 2020

ZhangYiJiang Dec 3, 2020

taneliang Dec 3, 2020

ZhangYiJiang Dec 3, 2020

ZhangYiJiang Dec 3, 2020

taneliang Dec 3, 2020

taneliang Dec 3, 2020

taneliang commented Dec 3, 2020

		const [Kawaii] = useState(() => sample(icons) ?? icons[0]);
		const [defaultMood] = useState(() => sample(defaultMoods) ?? defaultMoods[0]);

		draft.moduleBank.moduleList = (storeOverrides.moduleBank?.moduleList ??
		relevantStoreContents.moduleBank.moduleList) as typeof draft.moduleBank.moduleList;

Remove withRouter (1/n) #3007

Remove withRouter (1/n) #3007

Conversation

taneliang commented Dec 1, 2020

Context

Implementation

codecov bot commented Dec 1, 2020 • edited Loading

Codecov Report

nusmods-deploy-bot bot commented Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhangYiJiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taneliang Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taneliang commented Dec 3, 2020

Remove `withRouter` (1/n) #3007

Remove `withRouter` (1/n) #3007

codecov bot commented Dec 1, 2020 •

edited

Loading

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

edited

Loading

taneliang Dec 2, 2020 •

edited

Loading