From 58633350dabbc3056578a8e0f105fb809682fea4 Mon Sep 17 00:00:00 2001 From: Ian MacFarland Date: Wed, 9 Dec 2020 10:48:50 -0800 Subject: [PATCH 1/4] create store classes --- .../src/DataStore/DataStore.test.ts | 22 ++++++++++++++++ spotlight-client/src/DataStore/RootStore.ts | 26 +++++++++++++++++++ spotlight-client/src/DataStore/TenantStore.ts | 26 +++++++++++++++++++ spotlight-client/src/DataStore/index.ts | 20 ++++++++++++++ 4 files changed, 94 insertions(+) create mode 100644 spotlight-client/src/DataStore/DataStore.test.ts create mode 100644 spotlight-client/src/DataStore/RootStore.ts create mode 100644 spotlight-client/src/DataStore/TenantStore.ts create mode 100644 spotlight-client/src/DataStore/index.ts diff --git a/spotlight-client/src/DataStore/DataStore.test.ts b/spotlight-client/src/DataStore/DataStore.test.ts new file mode 100644 index 00000000..923fe9ed --- /dev/null +++ b/spotlight-client/src/DataStore/DataStore.test.ts @@ -0,0 +1,22 @@ +// Recidiviz - a data platform for criminal justice reform +// Copyright (C) 2020 Recidiviz, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . +// ============================================================================= + +import DataStore from "."; + +test("contains a tenant store", () => { + expect(DataStore?.tenantStore).toBeDefined(); +}); diff --git a/spotlight-client/src/DataStore/RootStore.ts b/spotlight-client/src/DataStore/RootStore.ts new file mode 100644 index 00000000..5003a46f --- /dev/null +++ b/spotlight-client/src/DataStore/RootStore.ts @@ -0,0 +1,26 @@ +// Recidiviz - a data platform for criminal justice reform +// Copyright (C) 2020 Recidiviz, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . +// ============================================================================= + +import TenantStore from "./TenantStore"; + +export default class RootStore { + tenantStore: TenantStore; + + constructor() { + this.tenantStore = new TenantStore({ rootStore: this }); + } +} diff --git a/spotlight-client/src/DataStore/TenantStore.ts b/spotlight-client/src/DataStore/TenantStore.ts new file mode 100644 index 00000000..d59faa2e --- /dev/null +++ b/spotlight-client/src/DataStore/TenantStore.ts @@ -0,0 +1,26 @@ +// Recidiviz - a data platform for criminal justice reform +// Copyright (C) 2020 Recidiviz, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . +// ============================================================================= + +import type RootStore from "./RootStore"; + +export default class TenantStore { + rootStore: RootStore; + + constructor({ rootStore }: { rootStore: RootStore }) { + this.rootStore = rootStore; + } +} diff --git a/spotlight-client/src/DataStore/index.ts b/spotlight-client/src/DataStore/index.ts new file mode 100644 index 00000000..7e501957 --- /dev/null +++ b/spotlight-client/src/DataStore/index.ts @@ -0,0 +1,20 @@ +// Recidiviz - a data platform for criminal justice reform +// Copyright (C) 2020 Recidiviz, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . +// ============================================================================= + +import RootStore from "./RootStore"; + +export default new RootStore(); From c4eea7516d2f4e71ac8ce1b0226da956f226b22d Mon Sep 17 00:00:00 2001 From: Ian MacFarland Date: Wed, 9 Dec 2020 11:10:38 -0800 Subject: [PATCH 2/4] add reactivity and tests --- spotlight-client/package.json | 1 + .../src/DataStore/DataStore.test.ts | 28 +++++++++++++++++-- spotlight-client/src/DataStore/TenantStore.ts | 11 ++++++++ yarn.lock | 5 ++++ 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/spotlight-client/package.json b/spotlight-client/package.json index 75da8c5b..4b512442 100644 --- a/spotlight-client/package.json +++ b/spotlight-client/package.json @@ -19,6 +19,7 @@ "@types/react-dom": "^16.9.0", "assert-never": "^1.2.1", "jest-fetch-mock": "^3.0.3", + "mobx": "^6.0.4", "react": "^16.13.1", "react-app-polyfill": "^1.0.6", "react-dom": "^16.13.1", diff --git a/spotlight-client/src/DataStore/DataStore.test.ts b/spotlight-client/src/DataStore/DataStore.test.ts index 923fe9ed..05effe6d 100644 --- a/spotlight-client/src/DataStore/DataStore.test.ts +++ b/spotlight-client/src/DataStore/DataStore.test.ts @@ -15,8 +15,32 @@ // along with this program. If not, see . // ============================================================================= -import DataStore from "."; +import Tenant from "../contentModels/Tenant"; +import RootStore from "./RootStore"; + +let DataStore: RootStore; + +beforeEach(() => { + DataStore = new RootStore(); +}); test("contains a tenant store", () => { - expect(DataStore?.tenantStore).toBeDefined(); + expect(DataStore.tenantStore).toBeDefined(); +}); + +describe("tenant store", () => { + let tenantStore: typeof DataStore.tenantStore; + + beforeEach(() => { + tenantStore = DataStore.tenantStore; + }); + + test("has no default tenant", () => { + expect(tenantStore.currentTenant).toBeUndefined(); + }); + + test("can set current tenant", () => { + tenantStore.setCurrentTenant({ tenantId: "US_ND" }); + expect(tenantStore.currentTenant).toBeInstanceOf(Tenant); + }); }); diff --git a/spotlight-client/src/DataStore/TenantStore.ts b/spotlight-client/src/DataStore/TenantStore.ts index d59faa2e..a4c1191f 100644 --- a/spotlight-client/src/DataStore/TenantStore.ts +++ b/spotlight-client/src/DataStore/TenantStore.ts @@ -15,12 +15,23 @@ // along with this program. If not, see . // ============================================================================= +import { makeAutoObservable } from "mobx"; +import { TenantId } from "../contentApi/types"; +import Tenant, { createTenant } from "../contentModels/Tenant"; import type RootStore from "./RootStore"; export default class TenantStore { + currentTenant?: Tenant; + rootStore: RootStore; constructor({ rootStore }: { rootStore: RootStore }) { + makeAutoObservable(this, { rootStore: false }); + this.rootStore = rootStore; } + + setCurrentTenant(opts: { tenantId: TenantId }): void { + this.currentTenant = createTenant(opts); + } } diff --git a/yarn.lock b/yarn.lock index bee70b1f..4ae17250 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8977,6 +8977,11 @@ mkdirp@^0.5.1, mkdirp@^0.5.3, mkdirp@^0.5.5, mkdirp@~0.5.1: dependencies: minimist "^1.2.5" +mobx@^6.0.4: + version "6.0.4" + resolved "https://registry.yarnpkg.com/mobx/-/mobx-6.0.4.tgz#8fc3e3629a3346f8afddf5bd954411974744dad1" + integrity sha512-wT2QJT9tW19VSHo9x7RPKU3z/I2Ps6wUS8Kb1OO+kzmg7UY3n4AkcaYG6jq95Lp1R9ohjC/NGYuT2PtuvBjhFg== + morgan@^1.10.0: version "1.10.0" resolved "https://registry.yarnpkg.com/morgan/-/morgan-1.10.0.tgz#091778abc1fc47cd3509824653dae1faab6b17d7" From d2f36be0cfc62576a4f61a6d2bcb3e282b32ad04 Mon Sep 17 00:00:00 2001 From: Ian MacFarland Date: Wed, 9 Dec 2020 11:47:38 -0800 Subject: [PATCH 3/4] make mutable model properties observable and others readonly --- .../src/contentModels/Collection.ts | 6 ++-- spotlight-client/src/contentModels/Metric.ts | 35 ++++++++++++------- spotlight-client/src/contentModels/Tenant.ts | 8 ++--- 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/spotlight-client/src/contentModels/Collection.ts b/spotlight-client/src/contentModels/Collection.ts index a96817e9..d1b47a7b 100644 --- a/spotlight-client/src/contentModels/Collection.ts +++ b/spotlight-client/src/contentModels/Collection.ts @@ -36,11 +36,11 @@ type InitOptions = { * needed for assembling known `Collection` kinds. */ export default class Collection { - description: string; + readonly description: string; - name: string; + readonly name: string; - metrics: MetricMapping; + readonly metrics: MetricMapping; constructor({ name, description, metrics }: InitOptions) { this.name = name; diff --git a/spotlight-client/src/contentModels/Metric.ts b/spotlight-client/src/contentModels/Metric.ts index 26c90e32..ff774ef8 100644 --- a/spotlight-client/src/contentModels/Metric.ts +++ b/spotlight-client/src/contentModels/Metric.ts @@ -16,6 +16,7 @@ // ============================================================================= import assertNever from "assert-never"; +import { makeAutoObservable, runInAction } from "mobx"; import { MetricTypeIdList, TenantContent, TenantId } from "../contentApi/types"; import { DemographicsByCategoryRecord, @@ -70,23 +71,23 @@ type InitOptions = { */ export default class Metric { // metadata properties - description: string; + readonly description: string; - methodology: string; + readonly methodology: string; - name: string; + readonly name: string; // relationships collections: CollectionMap = new Map(); // we don't really need the entire Tenant object, // only the ID for use in the API request - tenantId: TenantId; + readonly tenantId: TenantId; // data properties - private dataTransformer: DataTransformer; + private readonly dataTransformer: DataTransformer; - private sourceFileName: string; + private readonly sourceFileName: string; isLoading?: boolean; @@ -102,6 +103,14 @@ export default class Metric { dataTransformer, sourceFileName, }: InitOptions) { + makeAutoObservable(this, { + // readonly properties do not need to be observed + description: false, + methodology: false, + name: false, + tenantId: false, + }); + // initialize metadata this.name = name; this.description = description; @@ -119,13 +128,15 @@ export default class Metric { metricNames: [this.sourceFileName], tenantId: this.tenantId, }); - if (apiResponse) { - const metricFileData = apiResponse[this.sourceFileName]; - if (metricFileData) { - this.allRecords = this.dataTransformer(metricFileData); + runInAction(() => { + if (apiResponse) { + const metricFileData = apiResponse[this.sourceFileName]; + if (metricFileData) { + this.allRecords = this.dataTransformer(metricFileData); + } + this.isLoading = false; } - this.isLoading = false; - } + }); } get records(): RecordFormat[] | undefined { diff --git a/spotlight-client/src/contentModels/Tenant.ts b/spotlight-client/src/contentModels/Tenant.ts index f1b35be8..9c404ec6 100644 --- a/spotlight-client/src/contentModels/Tenant.ts +++ b/spotlight-client/src/contentModels/Tenant.ts @@ -22,10 +22,10 @@ import { createMetricMapping } from "./Metric"; import { CollectionMap, MetricMapping } from "./types"; type InitOptions = { - name: string; - description: string; - collections: CollectionMap; - metrics: MetricMapping; + readonly name: string; + readonly description: string; + readonly collections: CollectionMap; + readonly metrics: MetricMapping; }; /** From 06551f4909d94f5b6f5bffa99fd594000dbd14ff Mon Sep 17 00:00:00 2001 From: Ian MacFarland Date: Wed, 9 Dec 2020 16:42:46 -0800 Subject: [PATCH 4/4] make tests reactive --- spotlight-client/package.json | 1 + .../src/DataStore/DataStore.test.ts | 20 +++++- .../src/contentModels/Metric.test.ts | 65 ++++++++++++++----- spotlight-client/src/contentModels/Metric.ts | 4 ++ spotlight-client/src/index.tsx | 11 ++++ spotlight-client/src/setupTests.ts | 11 +++- spotlight-client/tsconfig.json | 3 +- yarn.lock | 5 ++ 8 files changed, 99 insertions(+), 21 deletions(-) diff --git a/spotlight-client/package.json b/spotlight-client/package.json index 4b512442..e44cd6f8 100644 --- a/spotlight-client/package.json +++ b/spotlight-client/package.json @@ -20,6 +20,7 @@ "assert-never": "^1.2.1", "jest-fetch-mock": "^3.0.3", "mobx": "^6.0.4", + "mobx-utils": "^6.0.1", "react": "^16.13.1", "react-app-polyfill": "^1.0.6", "react-dom": "^16.13.1", diff --git a/spotlight-client/src/DataStore/DataStore.test.ts b/spotlight-client/src/DataStore/DataStore.test.ts index 05effe6d..b3d542d1 100644 --- a/spotlight-client/src/DataStore/DataStore.test.ts +++ b/spotlight-client/src/DataStore/DataStore.test.ts @@ -15,11 +15,21 @@ // along with this program. If not, see . // ============================================================================= +import { autorun } from "mobx"; import Tenant from "../contentModels/Tenant"; import RootStore from "./RootStore"; let DataStore: RootStore; +/** + * Convenience method to run an immediate, one-time reactive effect + */ +function reactImmediately(effect: () => void) { + // this will call the effect function immediately, + // and then immediately call the disposer to tear down the reaction + autorun(effect)(); +} + beforeEach(() => { DataStore = new RootStore(); }); @@ -36,11 +46,17 @@ describe("tenant store", () => { }); test("has no default tenant", () => { - expect(tenantStore.currentTenant).toBeUndefined(); + reactImmediately(() => { + expect(tenantStore.currentTenant).toBeUndefined(); + }); + expect.hasAssertions(); }); test("can set current tenant", () => { tenantStore.setCurrentTenant({ tenantId: "US_ND" }); - expect(tenantStore.currentTenant).toBeInstanceOf(Tenant); + reactImmediately(() => { + expect(tenantStore.currentTenant).toBeInstanceOf(Tenant); + }); + expect.hasAssertions(); }); }); diff --git a/spotlight-client/src/contentModels/Metric.test.ts b/spotlight-client/src/contentModels/Metric.test.ts index a7bc5543..acb90abb 100644 --- a/spotlight-client/src/contentModels/Metric.test.ts +++ b/spotlight-client/src/contentModels/Metric.test.ts @@ -16,6 +16,8 @@ // ============================================================================= import fetchMock from "jest-fetch-mock"; +import { when } from "mobx"; +import { fromPromise } from "mobx-utils"; import { createMetricMapping } from "./Metric"; import retrieveContent from "../contentApi/retrieveContent"; import { MetricTypeId, MetricTypeIdList } from "../contentApi/types"; @@ -65,34 +67,63 @@ describe("data fetching", () => { testMetricMapping = getTestMapping(); }); - test.each(MetricTypeIdList)("for metric %s", async (metricId) => { + test.each(MetricTypeIdList)("for metric %s", (metricId, done) => { + expect.hasAssertions(); const metric = getTestMetric(metricId); - await metric.fetch(); - - // Be advised, these snapshots are huge! However, the only expected failure cases here are: - // 1. you intentionally changed the contents of the fixture in spotlight-api - // 2. you intentionally changed the record format for this Metric type - // Be especially careful inspecting snapshots for Metrics that filter their sources, - // e.g. Parole/Probation metrics. Verify that they use the right rows! - expect(metric.records).toMatchSnapshot(); + metric.fetch(); + + when( + () => metric.records !== undefined, + () => { + // Be advised, these snapshots are huge! However, the only expected failure cases here are: + // 1. you intentionally changed the contents of the fixture in spotlight-api + // 2. you intentionally changed the record format for this Metric type + // Be especially careful inspecting snapshots for Metrics that filter their sources, + // e.g. Parole/Probation metrics. Verify that they use the right rows! + expect(metric.records).toMatchSnapshot(); + // @ts-expect-error typedefs for `test.each` are wrong, `done` will be a function + done(); + } + ); }); }); -test("file loading state", async () => { +test("file loading state", (done) => { testMetricMapping = getTestMapping(); // not really necessary to test this once per type; we just pick one arbitrarily const metric = getTestMetric("ParoleSuccessHistorical"); - expect(metric.isLoading).toBeUndefined(); + let dataPromise: ReturnType; - const dataPromise = metric.fetch(); - expect(metric.isLoading).toBe(true); - expect(metric.records).toBeUndefined(); + // this should be the initial state of the metric instance + when( + () => metric.isLoading === undefined, + () => { + expect(metric.records).toBeUndefined(); + // the fetch is initiated here; this will trigger the reactions below + dataPromise = fromPromise(metric.fetch()); + } + ); + + when( + () => dataPromise.state === "pending", + () => { + expect(metric.isLoading).toBe(true); + expect(metric.records).toBeUndefined(); + } + ); + + when( + () => dataPromise.state === "fulfilled", + () => { + expect(metric.isLoading).toBe(false); + expect(metric.records).toBeDefined(); + done(); + } + ); - await dataPromise; - expect(metric.isLoading).toBe(false); - expect(metric.records).toBeDefined(); + expect.assertions(5); }); test("fetch error state", async () => { diff --git a/spotlight-client/src/contentModels/Metric.ts b/spotlight-client/src/contentModels/Metric.ts index ff774ef8..5bed75cd 100644 --- a/spotlight-client/src/contentModels/Metric.ts +++ b/spotlight-client/src/contentModels/Metric.ts @@ -109,6 +109,10 @@ export default class Metric { methodology: false, name: false, tenantId: false, + // in practice, collections should not change once we are done + // boostrapping them (which is done right after instantiation); + // no need to make them observable + collections: false, }); // initialize metadata diff --git a/spotlight-client/src/index.tsx b/spotlight-client/src/index.tsx index fab86209..c5c29098 100644 --- a/spotlight-client/src/index.tsx +++ b/spotlight-client/src/index.tsx @@ -17,10 +17,21 @@ import "react-app-polyfill/ie11"; import "react-app-polyfill/stable"; +import { configure } from "mobx"; + import React from "react"; import ReactDOM from "react-dom"; import App from "./App"; +configure({ + // make proxies optional for IE 11 support + useProxies: "ifavailable", + // activate runtime linting + computedRequiresReaction: true, + reactionRequiresObservable: true, + observableRequiresReaction: true, +}); + ReactDOM.render( diff --git a/spotlight-client/src/setupTests.ts b/spotlight-client/src/setupTests.ts index 1eebbc07..efc869e7 100644 --- a/spotlight-client/src/setupTests.ts +++ b/spotlight-client/src/setupTests.ts @@ -21,11 +21,20 @@ // learn more: https://github.com/testing-library/jest-dom import "@testing-library/jest-dom/extend-expect"; - import fetchMock from "jest-fetch-mock"; +import { configure } from "mobx"; // we want this mock to be available but disabled by default; // tests should default to doing real fetches against a /spotlight-api test server // but can mock it per test to simulate errors, etc fetchMock.enableMocks(); fetchMock.dontMock(); + +configure({ + // activate runtime linting + computedRequiresReaction: true, + reactionRequiresObservable: true, + observableRequiresReaction: true, + // debug setting to avoid silent failures in reactive code + disableErrorBoundaries: true, +}); diff --git a/spotlight-client/tsconfig.json b/spotlight-client/tsconfig.json index ca3e4c93..a04fff01 100644 --- a/spotlight-client/tsconfig.json +++ b/spotlight-client/tsconfig.json @@ -21,7 +21,8 @@ "isolatedModules": true, "lib": ["dom", "dom.iterable", "esnext"], "moduleResolution": "node", - "noEmit": true + "noEmit": true, + "useDefineForClassFields": true }, "include": ["src"] } diff --git a/yarn.lock b/yarn.lock index 4ae17250..5e86a460 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8977,6 +8977,11 @@ mkdirp@^0.5.1, mkdirp@^0.5.3, mkdirp@^0.5.5, mkdirp@~0.5.1: dependencies: minimist "^1.2.5" +mobx-utils@^6.0.1: + version "6.0.1" + resolved "https://registry.yarnpkg.com/mobx-utils/-/mobx-utils-6.0.1.tgz#bc16c2c62794a48498685f3663e6162d42fb6106" + integrity sha512-0p5xqHDt/arz3cswVcEHYKY/BwL+fifsNroT3Gjkz9lNC7ZDsJm3IuGsmYlvFcVRJRr1rwlFZGuZXrt/MhHEJQ== + mobx@^6.0.4: version "6.0.4" resolved "https://registry.yarnpkg.com/mobx/-/mobx-6.0.4.tgz#8fc3e3629a3346f8afddf5bd954411974744dad1"