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.

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

Create reactive datastore for models with Mobx #277

Merged
merged 12 commits into from
Dec 15, 2020
2 changes: 2 additions & 0 deletions spotlight-client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
"@types/react-dom": "^16.9.0",
"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",
Expand Down
62 changes: 62 additions & 0 deletions spotlight-client/src/DataStore/DataStore.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// 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 <https://www.gnu.org/licenses/>.
// =============================================================================

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

test("contains a tenant store", () => {
expect(DataStore.tenantStore).toBeDefined();
});

describe("tenant store", () => {
let tenantStore: typeof DataStore.tenantStore;

beforeEach(() => {
tenantStore = DataStore.tenantStore;
});

test("has no default tenant", () => {
reactImmediately(() => {
expect(tenantStore.currentTenant).toBeUndefined();
});
expect.hasAssertions();
});

test("can set current tenant", () => {
tenantStore.setCurrentTenant({ tenantId: "US_ND" });
reactImmediately(() => {
expect(tenantStore.currentTenant).toBeInstanceOf(Tenant);
});
expect.hasAssertions();
});
});
26 changes: 26 additions & 0 deletions spotlight-client/src/DataStore/RootStore.ts
Original file line number Diff line number Diff line change
@@ -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 <https://www.gnu.org/licenses/>.
// =============================================================================

import TenantStore from "./TenantStore";

export default class RootStore {
tenantStore: TenantStore;

constructor() {
this.tenantStore = new TenantStore({ rootStore: this });
}
}
37 changes: 37 additions & 0 deletions spotlight-client/src/DataStore/TenantStore.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// 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 <https://www.gnu.org/licenses/>.
// =============================================================================

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);
}
}
20 changes: 20 additions & 0 deletions spotlight-client/src/DataStore/index.ts
Original file line number Diff line number Diff line change
@@ -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 <https://www.gnu.org/licenses/>.
// =============================================================================

import RootStore from "./RootStore";

export default new RootStore();
6 changes: 3 additions & 3 deletions spotlight-client/src/contentModels/Collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
65 changes: 48 additions & 17 deletions spotlight-client/src/contentModels/Metric.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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<typeof fromPromise>;

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

Choose a reason for hiding this comment

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

Great tests. Smart to use when reaction for these tests!

});

test("fetch error state", async () => {
Expand Down
39 changes: 27 additions & 12 deletions spotlight-client/src/contentModels/Metric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// =============================================================================

import assertNever from "assert-never";
import { makeAutoObservable, runInAction } from "mobx";
import { MetricTypeIdList, TenantContent, TenantId } from "../contentApi/types";
import {
DemographicsByCategoryRecord,
Expand Down Expand Up @@ -70,23 +71,23 @@ type InitOptions<RecordFormat> = {
*/
export default class Metric<RecordFormat extends AnyRecord> {
// 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<RecordFormat>;
private readonly dataTransformer: DataTransformer<RecordFormat>;

private sourceFileName: string;
private readonly sourceFileName: string;

isLoading?: boolean;

Expand All @@ -102,6 +103,18 @@ export default class Metric<RecordFormat extends AnyRecord> {
dataTransformer,
sourceFileName,
}: InitOptions<RecordFormat>) {
makeAutoObservable(this, {
// readonly properties do not need to be observed
description: false,
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
this.name = name;
this.description = description;
Expand All @@ -119,13 +132,15 @@ export default class Metric<RecordFormat extends AnyRecord> {
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;

Choose a reason for hiding this comment

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

I'm trying to wrap my head around when it would make sense to make a store vs making observable attributes on a model like you've done here.

It makes sense that you would want isLoading to be observable so that components can change what they render based on this status boolean. Why did you choose to just set this in an action here vs creating a store and setting this in the store like you did for the TenantStore.currentTenant?

Choose a reason for hiding this comment

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

Is your intention to eventually store the metric data as part of the TenantStore using getMetricsForTenant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did it this way because the relationship between tenant and metric is strictly hierarchical ... the Mobx docs advise against normalizing those kinds of relationships or making your stores resemble relational database tables or something like that. Instead the metrics are literally nested within TenantStore.currentTenant.

As to your second question, it's a good question ... I don't really know the answer yet. There are some places where a currentMetric would make sense (e.g. you have navigated to the page for a single metric), but in other places you will see multiple metrics at once. There is nothing nested below a Metric so it wouldn't serve the same limiting function to designate one as "current" and thus it might not even be necessary to do it at all? Figuring this out will be a big part of #274.

}
this.isLoading = false;
}
});
}

get records(): RecordFormat[] | undefined {
Expand Down
8 changes: 4 additions & 4 deletions spotlight-client/src/contentModels/Tenant.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

/**
Expand Down
11 changes: 11 additions & 0 deletions spotlight-client/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<React.StrictMode>
<App />
Expand Down
Loading