Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Oct 24, 2022
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2fd5d84
WIP
mikelehen Oct 24, 2022
8632285
WIP
mikelehen Oct 24, 2022
01e853d
Rework metric hooks using SWR, and update tests.
mikelehen Oct 24, 2022
3c38025
CR feedback: use toBeUndefined() and toBeDefined().
mikelehen Oct 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/large-seas-shave.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@actnowcoalition/ui-components": patch
---

Rework metric data fetching hooks using SWR. Fixes #315.
2 changes: 1 addition & 1 deletion jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/** @type {import('ts-jest/dist/types').InitialOptionsTsJest} */
module.exports = {
preset: "ts-jest",
testEnvironment: "node",
testEnvironment: "jsdom",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it should be fine :)

moduleNameMapper: {
"^@actnowcoalition/(.*)$": "<rootDir>/packages/$1/src",
},
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"eslint-plugin-react-hooks": "^4.6.0",
"husky": "^8.0.1",
"jest": "^28.1.0",
"jest-environment-jsdom": "^29.2.2",
"lint-staged": "^12.4.1",
"lodash": "^4.17.21",
"plop": "^3.1.0",
Expand Down
3 changes: 2 additions & 1 deletion packages/ui-components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"@types/react-copy-to-clipboard": "^5.0.4",
"@visx/mock-data": "^2.1.2",
"babel-loader": "^8.2.5",
Expand Down Expand Up @@ -77,6 +77,7 @@
"react-share": "^4.4.0",
"react-simple-maps": "^3.0.0",
"react-singleton-hook": "^4.0.0",
"swr": "^1.3.0",
"topojson-client": "^3.1.0"
}
}
60 changes: 34 additions & 26 deletions packages/ui-components/src/common/hooks/metric-data.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import { renderHook } from "@testing-library/react-hooks";
import { renderHook, waitFor } from "@testing-library/react";

import {
MetricCatalog,
MetricData,
MultiMetricDataStore,
MultiRegionMultiMetricDataStore,
StaticValueDataProvider,
} from "@actnowcoalition/metrics";
import { states } from "@actnowcoalition/regions";
import {
DataOrError,
useData,
useDataForMetrics,
useDataForRegionsAndMetrics,
Expand Down Expand Up @@ -53,10 +49,7 @@ describe("metric data hooks", () => {
expect(catalog.dataFetchesCount).toBe(expectedFetches);

// Render the hook initially.
const { result, waitForNextUpdate, rerender } = renderHook<
any,
DataOrError<MultiRegionMultiMetricDataStore>
>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const { result, rerender } = renderHook(
({ regions, metrics }) =>
useDataForRegionsAndMetrics(
regions,
Expand All @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by change for consistency. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

// Wait for initial async fetch to finish.
await waitForNextUpdate();
await waitFor(() => {
expect(result.current.data).not.toBe(undefined);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

expect(
result.current.data?.metricData(testRegionCA, MetricId.PI).currentValue
).toBe(Math.PI);
Expand All @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Wait for async fetch to return the new data.
await waitForNextUpdate();
await waitFor(() => {
expect(result.current.data).not.toBe(undefined);
});
expect(result.current.data?.hasRegionData(testRegionCA)).toBe(false);
expect(result.current.data?.hasMetricData(testRegionWA, MetricId.PI)).toBe(
false
Expand All @@ -112,10 +112,7 @@ describe("metric data hooks", () => {
expect(catalog.dataFetchesCount).toBe(expectedFetches);

// Render the hook initially.
const { result, waitForNextUpdate, rerender } = renderHook<
any,
DataOrError<MultiMetricDataStore>
>(
const { result, rerender } = renderHook(
({ region, metrics }) =>
useDataForMetrics(region, metrics, /*includeTimeseries=*/ true),
{
Expand All @@ -130,9 +127,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);
// Wait for initial async fetch to finish.
await waitForNextUpdate();
await waitFor(() => {
expect(result.current.data).not.toBe(undefined);
});
expect(result.current.data?.metricData(MetricId.PI).currentValue).toBe(
Math.PI
);
Expand All @@ -148,8 +147,13 @@ describe("metric data hooks", () => {
// Render the hook again with different metrics. This should perform a fetch.
rerender({ region: testRegionCA, metrics: [MetricId.E] });
expect(catalog.dataFetchesCount).toBe(++expectedFetches);
// Data should go back to loading state.
expect(result.current.data).toBe(undefined);

// Wait for async fetch to return the new data.
await waitForNextUpdate();
await waitFor(() => {
expect(result.current.data).not.toBe(undefined);
});
expect(result.current.data?.hasMetricData(MetricId.PI)).toBe(false);
expect(result.current.data?.metricData(MetricId.E).currentValue).toBe(
Math.E
Expand All @@ -164,10 +168,7 @@ describe("metric data hooks", () => {
expect(catalog.dataFetchesCount).toBe(expectedFetches);

// Render the hook initially.
const { result, waitForNextUpdate, rerender } = renderHook<
any,
DataOrError<MetricData>
>(
const { result, rerender } = renderHook(
({ region, metric }) =>
useData(region, metric, /*includeTimeseries=*/ true),
{
Expand All @@ -182,16 +183,23 @@ 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);
// Wait for initial async fetch to finish.
await waitForNextUpdate();
await waitFor(() => {
expect(result.current.data).not.toBe(undefined);
});
expect(result.current.data?.currentValue).toBe(Math.PI);

// Render the hook again with a different metric. This should perform a fetch.
rerender({ region: testRegionCA, metric: MetricId.E });
expect(catalog.dataFetchesCount).toBe(++expectedFetches);
// Data should go back to loading state.
expect(result.current.data).toBe(undefined);

// Wait for async fetch to return the new data.
await waitForNextUpdate();
await waitFor(() => {
expect(result.current.data).not.toBe(undefined);
});
expect(result.current.data?.currentValue).toBe(Math.E);
});
});
Expand Down
65 changes: 15 additions & 50 deletions packages/ui-components/src/common/hooks/metric-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import useSWR from "swr";
import { Region } from "@actnowcoalition/regions";
import {
Metric,
Expand All @@ -6,8 +7,6 @@ import {
MultiRegionMultiMetricDataStore,
} from "@actnowcoalition/metrics";
import { useMetricCatalog } from "../../components/MetricCatalogContext";
import { useCallback, useEffect, useState } from "react";
import { useCachedArrayIfEqual } from "./useCachedArrayIfEqual";

/**
* Used as the result of a React hook in order to represent one of three states:
Expand All @@ -32,12 +31,13 @@ export function useData(
includeTimeseries = false
): DataOrError<MetricData> {
const catalog = useMetricCatalog();
const args = { region, metric, includeTimeseries };

const fetchData = useCallback(() => {
const fetcher = ({ region, metric, includeTimeseries }: typeof args) => {
return catalog.fetchData(region, metric, includeTimeseries);
}, [catalog, region, metric, includeTimeseries]);
};

return useFetchedData(fetchData);
return useSWR(args, fetcher);
}

/**
Expand All @@ -54,21 +54,13 @@ export function useDataForMetrics(
includeTimeseries = false
): DataOrError<MultiMetricDataStore> {
const catalog = useMetricCatalog();
let resolvedMetrics = metrics.map((m) => catalog.getMetric(m));
const args = { region, metrics, includeTimeseries };

// 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

resolvedMetrics = useCachedArrayIfEqual(resolvedMetrics);
const fetchData = useCallback(() => {
return catalog.fetchDataForMetrics(
region,
resolvedMetrics,
includeTimeseries
);
}, [catalog, region, resolvedMetrics, includeTimeseries]);
const fetcher = ({ region, metrics, includeTimeseries }: typeof args) => {
return catalog.fetchDataForMetrics(region, metrics, includeTimeseries);
};

return useFetchedData(fetchData);
return useSWR(args, fetcher);
}

/**
Expand All @@ -85,41 +77,14 @@ export function useDataForRegionsAndMetrics(
includeTimeseries = false
): DataOrError<MultiRegionMultiMetricDataStore> {
const catalog = useMetricCatalog();
let resolvedMetrics = metrics.map((m) => catalog.getMetric(m));

// In order to allow people to pass in a new array of regions / metrics that
// contain the same regions / metrics as before without triggering additional
// fetches, we need this caching trick.
resolvedMetrics = useCachedArrayIfEqual(resolvedMetrics);
regions = useCachedArrayIfEqual(regions);
const fetchData = useCallback(() => {
const args = { regions, metrics, includeTimeseries };
const fetcher = ({ regions, metrics, includeTimeseries }: typeof args) => {
return catalog.fetchDataForRegionsAndMetrics(
regions,
resolvedMetrics,
metrics,
includeTimeseries
);
}, [catalog, regions, resolvedMetrics, includeTimeseries]);

return useFetchedData(fetchData);
}

/**
* Helper to implement the useData*() hooks. It calls the provided fetchData()
* callback that returns a Promise and uses the result of the promise to
* populate the DataOrError result once the promise completes.
*/
function useFetchedData<T>(fetchData: () => Promise<T>): DataOrError<T> {
const [data, setData] = useState<T>();
};

useEffect(() => {
fetchData()
.then((result) => {
setData(result);
})
.catch((error) => {
console.error(`Error fetching metric data: ${error}`);
return { error };
});
}, [fetchData]);
return { data };
return useSWR(args, fetcher);
}
34 changes: 0 additions & 34 deletions packages/ui-components/src/common/hooks/useCachedArrayIfEqual.test.ts
View file
Open in desktop

This file was deleted.

23 changes: 0 additions & 23 deletions packages/ui-components/src/common/hooks/useCachedArrayIfEqual.ts
View file
Open in desktop

This file was deleted.

Loading