credExplorer gets adapters from one point (#786)

Currently, the `credExplorer` uses the `defaultStaticAdapters`, but it imports these adapters in multiple places. If we decide to make the adapters configurable (e.g. when we start supporting more plugins) this will be a problem. This change modifies the cred explorer so that the adapters always come from a prop declaration on the app. Then the adapters are passed into the `state` module's functional entry points, rather than letting `state` depend on the default adapters directly. This change is motivated by the fact that my WeightConfig cleanup can be done more cleanly if the adapters are present as a prop on the App. Test plan: Unit tests are updated. Also, `git grep defaultStaticAdapters` reaveals that the adapters are only consumed once.
2018-09-05 16:10:11 -07:00 · 2018-09-05 16:10:11 -07:00 · d77c76082d
parent f7383bbc90
commit d77c76082d
4 changed files with 88 additions and 30 deletions
--- a/src/app/credExplorer/App.js
+++ b/src/app/credExplorer/App.js
@ -9,7 +9,6 @@ import CheckedLocalStore from "../checkedLocalStore";
 import BrowserLocalStore from "../browserLocalStore";

 import {type EdgeEvaluator} from "../../core/attribution/pagerank";
-import {defaultStaticAdapters} from "../adapters/defaultPlugins";
 import {PagerankTable} from "./pagerankTable/Table";
 import {WeightConfig} from "./WeightConfig";
 import RepositorySelect from "./RepositorySelect";
@ -20,6 +19,8 @@ import {
  type StateTransitionMachineInterface,
  uninitializedState,
 } from "./state";
+import {StaticAdapterSet} from "../adapters/adapterSet";
+import {defaultStaticAdapters} from "../adapters/defaultPlugins";

 export default class AppPage extends React.Component<{|+assets: Assets|}> {
  static _LOCAL_STORE = new CheckedLocalStore(
@ -31,11 +32,21 @@ export default class AppPage extends React.Component<{|+assets: Assets|}> {

  render() {
    const App = createApp(createStateTransitionMachine);
-    return <App assets={this.props.assets} localStore={AppPage._LOCAL_STORE} />;
+    return (
+      <App
+        assets={this.props.assets}
+        adapters={defaultStaticAdapters()}
+        localStore={AppPage._LOCAL_STORE}
+      />
+    );
  }
 }

-type Props = {|+assets: Assets, +localStore: LocalStore|};
+type Props = {|
+  +assets: Assets,
+  +localStore: LocalStore,
+  +adapters: StaticAdapterSet,
+|};
 type State = {|
  appState: AppState,
  edgeEvaluator: ?EdgeEvaluator,
@ -110,6 +121,7 @@ export function createApp(
            onClick={() =>
              this.stateTransitionMachine.loadGraphAndRunPagerank(
                this.props.assets,
+                this.props.adapters,
                NullUtil.get(this.state.edgeEvaluator),
                GithubPrefix.user
              )
@ -119,7 +131,7 @@ export function createApp(
          </button>
          <WeightConfig
            onChange={(edgeEvaluator) => this.setState({edgeEvaluator})}
-            adapters={defaultStaticAdapters()}
+            adapters={this.props.adapters}
          />
          <LoadingIndicator appState={this.state.appState} />
          {pagerankTable}
--- a/src/app/credExplorer/App.test.js
+++ b/src/app/credExplorer/App.test.js
@ -38,7 +38,11 @@ describe("app/credExplorer/App", () => {
    }
    const App = createApp(createMockSTM);
    const el = shallow(
-      <App assets={new Assets("/foo/")} localStore={localStore} />
+      <App
+        assets={new Assets("/foo/")}
+        adapters={new StaticAdapterSet([])}
+        localStore={localStore}
+      />
    );
    if (setState == null || getState == null) {
      throw new Error("Initialization problems");
@ -156,6 +160,7 @@ describe("app/credExplorer/App", () => {
          expect(loadGraphAndRunPagerank).toBeCalledTimes(1);
          expect(loadGraphAndRunPagerank).toBeCalledWith(
            el.instance().props.assets,
+            el.instance().props.adapters,
            edgeEvaluator,
            GithubPrefix.user
          );
--- a/src/app/credExplorer/state.js
+++ b/src/app/credExplorer/state.js
@ -12,9 +12,7 @@ import {
  pagerank,
 } from "../../core/attribution/pagerank";

-import {DynamicAdapterSet} from "../adapters/adapterSet";
-
-import {defaultStaticAdapters} from "../adapters/defaultPlugins";
+import {StaticAdapterSet, DynamicAdapterSet} from "../adapters/adapterSet";

 /*
  This models the UI states of the credExplorer/App as a state machine.
@ -72,10 +70,11 @@ export function uninitializedState(): AppState {
 // Exported for testing purposes.
 export interface StateTransitionMachineInterface {
  +setRepo: (Repo) => void;
-  +loadGraph: (Assets) => Promise<boolean>;
+  +loadGraph: (Assets, StaticAdapterSet) => Promise<boolean>;
  +runPagerank: (EdgeEvaluator, NodeAddressT) => Promise<void>;
  +loadGraphAndRunPagerank: (
    Assets,
+    StaticAdapterSet,
    EdgeEvaluator,
    NodeAddressT
  ) => Promise<void>;
@ -89,6 +88,7 @@ export class StateTransitionMachine implements StateTransitionMachineInterface {
  setState: (AppState) => void;
  loadGraphWithAdapters: (
    assets: Assets,
+    adapters: StaticAdapterSet,
    repo: Repo
  ) => Promise<GraphWithAdapters>;
  pagerank: (
@ -102,6 +102,7 @@ export class StateTransitionMachine implements StateTransitionMachineInterface {
    setState: (AppState) => void,
    loadGraphWithAdapters: (
      assets: Assets,
+      adapters: StaticAdapterSet,
      repo: Repo
    ) => Promise<GraphWithAdapters>,
    pagerank: (
@ -126,7 +127,10 @@ export class StateTransitionMachine implements StateTransitionMachineInterface {
  }

  /** Loads the graph, reports whether it was successful */
-  async loadGraph(assets: Assets): Promise<boolean> {
+  async loadGraph(
+    assets: Assets,
+    adapters: StaticAdapterSet
+  ): Promise<boolean> {
    const state = this.getState();
    if (state.type !== "READY_TO_LOAD_GRAPH") {
      throw new Error("Tried to loadGraph in incorrect state");
@ -137,7 +141,11 @@ export class StateTransitionMachine implements StateTransitionMachineInterface {
    let newState: ?AppState;
    let success = true;
    try {
-      const graphWithAdapters = await this.loadGraphWithAdapters(assets, repo);
+      const graphWithAdapters = await this.loadGraphWithAdapters(
+        assets,
+        adapters,
+        repo
+      );
      newState = {
        type: "READY_TO_RUN_PAGERANK",
        graphWithAdapters,
@ -206,6 +214,7 @@ export class StateTransitionMachine implements StateTransitionMachineInterface {

  async loadGraphAndRunPagerank(
    assets: Assets,
+    adapters: StaticAdapterSet,
    edgeEvaluator: EdgeEvaluator,
    totalScoreNodePrefix: NodeAddressT
  ) {
@ -216,7 +225,7 @@ export class StateTransitionMachine implements StateTransitionMachineInterface {
    }
    switch (type) {
      case "READY_TO_LOAD_GRAPH":
-        const loadedGraph = await this.loadGraph(assets);
+        const loadedGraph = await this.loadGraph(assets, adapters);
        if (loadedGraph) {
          await this.runPagerank(edgeEvaluator, totalScoreNodePrefix);
        }
@ -237,8 +246,9 @@ export type GraphWithAdapters = {|
 |};
 export async function loadGraphWithAdapters(
  assets: Assets,
+  adapters: StaticAdapterSet,
  repo: Repo
 ): Promise<GraphWithAdapters> {
-  const adapters = await defaultStaticAdapters().load(assets, repo);
-  return {graph: adapters.graph(), adapters};
+  const dynamicAdapters = await adapters.load(assets, repo);
+  return {graph: dynamicAdapters.graph(), adapters: dynamicAdapters};
 }
--- a/src/app/credExplorer/state.test.js
+++ b/src/app/credExplorer/state.test.js
@ -26,6 +26,7 @@ describe("app/credExplorer/state", () => {
    };
    const loadGraphMock: (
      assets: Assets,
+      adapters: StaticAdapterSet,
      repo: Repo
    ) => Promise<GraphWithAdapters> = jest.fn();
    const pagerankMock: (
@ -136,25 +137,28 @@ describe("app/credExplorer/state", () => {
      ];
      for (const b of badStates) {
        const {stm} = example(b);
-        await expect(stm.loadGraph(new Assets("/my/gateway/"))).rejects.toThrow(
-          "incorrect state"
-        );
+        await expect(
+          stm.loadGraph(new Assets("/my/gateway/"), new StaticAdapterSet([]))
+        ).rejects.toThrow("incorrect state");
      }
    });
    it("passes along the adapters and repo", () => {
      const {stm, loadGraphMock} = example(readyToLoadGraph());
      expect(loadGraphMock).toHaveBeenCalledTimes(0);
      const assets = new Assets("/my/gateway/");
-      stm.loadGraph(assets);
+      const adapters = new StaticAdapterSet([]);
+      stm.loadGraph(assets, adapters);
      expect(loadGraphMock).toHaveBeenCalledTimes(1);
-      expect(loadGraphMock.mock.calls[0]).toHaveLength(2);
-      expect(loadGraphMock.mock.calls[0][0]).toBe(assets);
-      expect(loadGraphMock.mock.calls[0][1]).toEqual(makeRepo("foo", "bar"));
+      expect(loadGraphMock).toHaveBeenCalledWith(
+        assets,
+        adapters,
+        makeRepo("foo", "bar")
+      );
    });
    it("immediately sets loading status", () => {
      const {getState, stm} = example(readyToLoadGraph());
      expect(loading(getState())).toBe("NOT_LOADING");
-      stm.loadGraph(new Assets("/my/gateway/"));
+      stm.loadGraph(new Assets("/my/gateway/"), new StaticAdapterSet([]));
      expect(loading(getState())).toBe("LOADING");
      expect(getState().type).toBe("READY_TO_LOAD_GRAPH");
    });
@ -162,7 +166,10 @@ describe("app/credExplorer/state", () => {
      const {getState, stm, loadGraphMock} = example(readyToLoadGraph());
      const gwa = graphWithAdapters();
      loadGraphMock.mockResolvedValue(gwa);
-      const succeeded = await stm.loadGraph(new Assets("/my/gateway/"));
+      const succeeded = await stm.loadGraph(
+        new Assets("/my/gateway/"),
+        new StaticAdapterSet([])
+      );
      expect(succeeded).toBe(true);
      const state = getState();
      expect(loading(state)).toBe("NOT_LOADING");
@ -182,7 +189,10 @@ describe("app/credExplorer/state", () => {
            resolve(graphWithAdapters());
          })
      );
-      const succeeded = await stm.loadGraph(new Assets("/my/gateway/"));
+      const succeeded = await stm.loadGraph(
+        new Assets("/my/gateway/"),
+        new StaticAdapterSet([])
+      );
      expect(succeeded).toBe(false);
      const state = getState();
      expect(loading(state)).toBe("NOT_LOADING");
@ -195,7 +205,10 @@ describe("app/credExplorer/state", () => {
      // $ExpectFlowError
      console.error = jest.fn();
      loadGraphMock.mockRejectedValue(error);
-      const succeeded = await stm.loadGraph(new Assets("/my/gateway/"));
+      const succeeded = await stm.loadGraph(
+        new Assets("/my/gateway/"),
+        new StaticAdapterSet([])
+      );
      expect(succeeded).toBe(false);
      const state = getState();
      expect(loading(state)).toBe("FAILED");
@ -282,6 +295,7 @@ describe("app/credExplorer/state", () => {
      await expect(
        stm.loadGraphAndRunPagerank(
          new Assets("gateway"),
+          new StaticAdapterSet([]),
          edgeEvaluator(),
          NodeAddress.empty
        )
@ -293,11 +307,12 @@ describe("app/credExplorer/state", () => {
      (stm: any).runPagerank = jest.fn();
      stm.loadGraph.mockResolvedValue(true);
      const assets = new Assets("/gateway/");
+      const adapters = new StaticAdapterSet([]);
      const prefix = NodeAddress.fromParts(["bar"]);
      const ee = edgeEvaluator();
-      await stm.loadGraphAndRunPagerank(assets, ee, prefix);
+      await stm.loadGraphAndRunPagerank(assets, adapters, ee, prefix);
      expect(stm.loadGraph).toHaveBeenCalledTimes(1);
-      expect(stm.loadGraph).toHaveBeenCalledWith(assets);
+      expect(stm.loadGraph).toHaveBeenCalledWith(assets, adapters);
      expect(stm.runPagerank).toHaveBeenCalledTimes(1);
      expect(stm.runPagerank).toHaveBeenCalledWith(ee, prefix);
    });
@ -307,8 +322,14 @@ describe("app/credExplorer/state", () => {
      (stm: any).runPagerank = jest.fn();
      stm.loadGraph.mockResolvedValue(false);
      const assets = new Assets("/gateway/");
+      const adapters = new StaticAdapterSet([]);
      const prefix = NodeAddress.fromParts(["bar"]);
-      await stm.loadGraphAndRunPagerank(assets, edgeEvaluator(), prefix);
+      await stm.loadGraphAndRunPagerank(
+        assets,
+        adapters,
+        edgeEvaluator(),
+        prefix
+      );
      expect(stm.loadGraph).toHaveBeenCalledTimes(1);
      expect(stm.runPagerank).toHaveBeenCalledTimes(0);
    });
@ -318,7 +339,12 @@ describe("app/credExplorer/state", () => {
      (stm: any).runPagerank = jest.fn();
      const prefix = NodeAddress.fromParts(["bar"]);
      const ee = edgeEvaluator();
-      await stm.loadGraphAndRunPagerank(new Assets("/gateway/"), ee, prefix);
+      await stm.loadGraphAndRunPagerank(
+        new Assets("/gateway/"),
+        new StaticAdapterSet([]),
+        ee,
+        prefix
+      );
      expect(stm.loadGraph).toHaveBeenCalledTimes(0);
      expect(stm.runPagerank).toHaveBeenCalledTimes(1);
      expect(stm.runPagerank).toHaveBeenCalledWith(ee, prefix);
@ -329,7 +355,12 @@ describe("app/credExplorer/state", () => {
      (stm: any).runPagerank = jest.fn();
      const prefix = NodeAddress.fromParts(["bar"]);
      const ee = edgeEvaluator();
-      await stm.loadGraphAndRunPagerank(new Assets("/gateway/"), ee, prefix);
+      await stm.loadGraphAndRunPagerank(
+        new Assets("/gateway/"),
+        new StaticAdapterSet([]),
+        ee,
+        prefix
+      );
      expect(stm.loadGraph).toHaveBeenCalledTimes(0);
      expect(stm.runPagerank).toHaveBeenCalledTimes(1);
      expect(stm.runPagerank).toHaveBeenCalledWith(ee, prefix);