Rework metric hooks using SWR, and update tests. #327

mikelehen · 2022-10-24T22:13:10Z

Reworked metric data hooks to use useSWR hook from https://swr.vercel.app/. This was surprisingly smooth, and it fixes Revisit behavior of asynchronous useMetricData() hooks. #315 (the hook will now return to the loading state (data==undefined) when you change the metrics / regions you're asking for).
Reworked the tests to use @testing-library/react instead of @testing-library/react-hooks as the latter was triggering a is soon going to be deprecated and for some reason with SWR was triggering a "A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown." warning.
minor cleanup

changeset-bot · 2022-10-24T22:13:13Z

🦋 Changeset detected

Latest commit: 3c38025

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@actnowcoalition/ui-components	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2022-10-24T22:19:06Z

Visit the preview URL for this PR (updated for commit 3c38025):

https://act-now-packages--pr327-mikelehen-metric-hoo-n27yao2g.web.app

_{(expires Mon, 31 Oct 2022 23:59:24 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: ad39b6c29b2ae4e8fa7983fbf3316d2e514cb069}

mikelehen · 2022-10-24T22:14:25Z

jest.config.js

@@ -1,7 +1,7 @@
 /** @type {import('ts-jest/dist/types').InitialOptionsTsJest} */
 module.exports = {
  preset: "ts-jest",
-  testEnvironment: "node",
+  testEnvironment: "jsdom",


I had to do this after switching to @testing-library/react. The tests still pass, and I think jsdom probably makes sense for testing web components anyway, so hopefully it's fine? 🤷

yeah it should be fine :)

mikelehen · 2022-10-24T22:16:07Z

packages/ui-components/package.json

@@ -39,7 +39,7 @@
    "@storybook/manager-webpack4": "^6.5.12",
    "@storybook/react": "^6.5.12",
    "@storybook/testing-library": "^0.0.13",
-    "@testing-library/react-hooks": "^8.0.1",
+    "@testing-library/react": "^13.4.0",


I switched because react-hooks is going to be deprecated and this solved a "A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown" warning I was getting after I started using SWR.

mikelehen · 2022-10-24T22:17:08Z

packages/ui-components/src/common/hooks/metric-data.test.tsx

-    const { result, waitForNextUpdate, rerender } = renderHook<
-      any,
-      DataOrError<MultiRegionMultiMetricDataStore>
-    >(


There was something messed up with the @testing-library/react-hooks types that forced me to manually specify the generic types and use any. But with @testing-library/react this problem went away.

mikelehen · 2022-10-24T22:18:23Z

packages/ui-components/src/common/hooks/metric-data.test.tsx

-    await waitForNextUpdate();
+    await waitFor(() => {
+      expect(result.current.data).not.toBe(undefined);
+    });


@testing-library/react got rid of waitForNextUpdate() and this is the new way to do it, I guess. I don't love it, but it's what's in the migration guide.

mikelehen · 2022-10-24T22:19:14Z

packages/ui-components/src/common/hooks/metric-data.test.tsx

@@ -93,8 +88,13 @@ describe("metric data hooks", () => {
    // Render the hook again with different regions / metrics.  This should perform a fetch.
    rerender({ regions: [testRegionWA], metrics: [MetricId.E] });
    expect(catalog.dataFetchesCount).toBe(++expectedFetches);
+    // Data should go back to loading state.
+    expect(result.current.data).toBe(undefined);


This expectation is new, and it would fail before we switched to SWR. Now, when you request different regions / metrics, we temporarily revert to the loading state.

mikelehen · 2022-10-24T22:19:56Z

packages/ui-components/src/common/hooks/metric-data.test.tsx

@@ -75,9 +68,11 @@ describe("metric data hooks", () => {
    // The catalog should have performed a data fetch, but since it's anync,
    // initially the hook won't return data.
    expect(catalog.dataFetchesCount).toBe(++expectedFetches);
-    expect(result.current.data).toStrictEqual(undefined);
+    expect(result.current.data).toBe(undefined);


Drive-by change for consistency. 🤷

Alternatively, we could use toBeUndefined() and not.toBeUndefined() (the Jest docs say it's better practice, but they don't say why)

You could write expect(bestDrinkForFlavor('octopus')).toBe(undefined), but it's better practice to avoid referring to undefined directly in your code.

Ah! Interesting. That's probably a tad cleaner. And there's a toBeDefined() I can use instead of not.toBeUndefined() which I definitely like better. Updated!

FWIW- In the olden days of JavaScript, when you referenced undefined in your code, you were actually just referencing a global variable called undefined that had the value undefined and if you accidentally wrote code like this:

var globalToUpdate; window[globalToUpdate] = "hello";

You would accidentally overwrite the undefined variable and so now whenever you referred to undefined in your code, you'd end up with "hello" instead of undefined.

And so the "correct" way to check for undefined was typeof value === "undefined" instead of value === undefined.

But at some point browsers realized that was terrible and started prevented you from overwriting undefined. So window["test"] = "hello" will create a global called test, but window["undefined"] = "hello" is a no-op. 😁

I'm guessing that's part of why jest has those docs and it's not as relevant now. But toBeUndefined() is pretty clean, so I'll go ahead and use it!

mikelehen · 2022-10-24T22:21:52Z

packages/ui-components/src/common/hooks/metric-data.ts


-  // In order to allow people to pass in a new array of metrics that
-  // contain the same metrics as before without triggering additional
-  // fetches, we need this caching trick.


Note that I think SWR is going to serialize the list of metrics / regions in order to check if they changed between calls, which is slower than what I was doing here, but hopefully it's fine. (I'm mostly just nervous about when we do regions.all for fetching data for rendering the map / compare table, etc. Serializing the whole list of regions isn't ideal.)

* Reworked metric data hooks to use `useSWR` hook from https://swr.vercel.app/. This was surprisingly smooth, and it fixes #315 (the hook will now return to the loading state (data==undefined) when you change the metrics / regions you're asking for). * Reworked the tests to use `@testing-library/react` instead of `@testing-library/react-hooks` as the latter was triggering a is soon going to be deprecated and for some reason with SWR was triggering a "A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown." warning. * minor cleanup

pnavarrc

Looks good! Optionally we could use .toBeUndefined() for the tests, but I'm not sure if that's actually better.

pnavarrc · 2022-10-24T23:30:20Z

packages/ui-components/src/common/hooks/metric-data.test.tsx

@@ -75,9 +68,11 @@ describe("metric data hooks", () => {
    // The catalog should have performed a data fetch, but since it's anync,
    // initially the hook won't return data.
    expect(catalog.dataFetchesCount).toBe(++expectedFetches);
-    expect(result.current.data).toStrictEqual(undefined);
+    expect(result.current.data).toBe(undefined);


Alternatively, we could use toBeUndefined() and not.toBeUndefined() (the Jest docs say it's better practice, but they don't say why)

You could write expect(bestDrinkForFlavor('octopus')).toBe(undefined), but it's better practice to avoid referring to undefined directly in your code.

mikelehen commented Oct 24, 2022

View reviewed changes

mikelehen added 3 commits October 24, 2022 15:24

WIP

2fd5d84

WIP

8632285

mikelehen force-pushed the mikelehen/metric-hook-improvements-2 branch from e605e27 to 01e853d Compare October 24, 2022 22:25

mikelehen marked this pull request as ready for review October 24, 2022 22:25

pnavarrc approved these changes Oct 24, 2022

View reviewed changes

CR feedback: use toBeUndefined() and toBeDefined().

3c38025

mikelehen merged commit ffe37ea into develop Oct 24, 2022

mikelehen deleted the mikelehen/metric-hook-improvements-2 branch October 24, 2022 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework metric hooks using SWR, and update tests. #327

Rework metric hooks using SWR, and update tests. #327

mikelehen commented Oct 24, 2022

changeset-bot bot commented Oct 24, 2022 •

edited

Loading

github-actions bot commented Oct 24, 2022 •

edited

Loading

mikelehen Oct 24, 2022

pnavarrc Oct 24, 2022

mikelehen Oct 24, 2022

mikelehen Oct 24, 2022

mikelehen Oct 24, 2022

mikelehen Oct 24, 2022

mikelehen Oct 24, 2022

pnavarrc Oct 24, 2022

mikelehen Oct 24, 2022

mikelehen Oct 24, 2022

pnavarrc left a comment

pnavarrc Oct 24, 2022

Rework metric hooks using SWR, and update tests. #327

Rework metric hooks using SWR, and update tests. #327

Conversation

mikelehen commented Oct 24, 2022

changeset-bot bot commented Oct 24, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Oct 24, 2022 • 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

Choose a reason for hiding this comment

pnavarrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Oct 24, 2022 •

edited

Loading

github-actions bot commented Oct 24, 2022 •

edited

Loading