Make credExplorer/App control the WeightedTypes (#796)

This commit implements a [suggestion] to make `credExplorer/App` a
single source of truth on the `WeightedTypes`. As such, both
`WeightConfig` and `PluginWeightConfig` have been refactored to be
(essentially) stateless components that are controlled from above. I say
essentially because `WeightConfig` still has its expanded state, but
that will go away soon.

Along the way, I've improved testing and added some new invariant
checking (e.g. that `PluginWeightConfig` was passed the correct set of
weights for its adapter). For the first time, there are now tests for
the `WeightConfig` itself! I'm not totally done with the weight
re-write, but this seems like a good time to close #604, as the whole
logical sequence for setting weights is now tested.

Test plan: There are new unit tests. Also, to be sure, I manually tested
the UI as well.

[suggestion]: https://github.com/sourcecred/sourcecred/pull/792#issuecomment-419234721

src/app/credExplorer/App.js

@@ -132,6 +132,7 @@ export function createApp(
           </button>
           <WeightConfig
             onChange={(weightedTypes) => this.setState({weightedTypes})}
+            weightedTypes={this.state.weightedTypes}
             adapters={this.props.adapters}
           />
           <LoadingIndicator appState={this.state.appState} />

src/app/credExplorer/WeightConfig.js

@@ -2,40 +2,31 @@
 
 import React from "react";
 import * as NullUtil from "../../util/null";
+import * as MapUtil from "../../util/map";
 
-import type {StaticPluginAdapter} from "../adapters/pluginAdapter";
 import type {StaticAdapterSet} from "../adapters/adapterSet";
-import {
-  type WeightedTypes,
-  defaultWeightsForAdapter,
-  combineWeights,
-} from "./weights/weights";
+import type {WeightedTypes} from "./weights/weights";
 import {PluginWeightConfig} from "./weights/PluginWeightConfig";
 import {FALLBACK_NAME} from "../adapters/fallbackAdapter";
 
 type Props = {|
   +adapters: StaticAdapterSet,
+  +weightedTypes: WeightedTypes,
   +onChange: (WeightedTypes) => void,
 |};
 
-type State = {
-  pluginNameToWeights: Map<string, WeightedTypes>,
+type State = {|
   expanded: boolean,
-};
+|};
 
 export class WeightConfig extends React.Component<Props, State> {
   constructor(props: Props): void {
     super(props);
     this.state = {
-      pluginNameToWeights: new Map(),
       expanded: false,
     };
   }
 
-  componentDidMount() {
-    this.fire();
-  }
-
   render() {
     const {expanded} = this.state;
     return (
@@ -55,43 +46,55 @@ export class WeightConfig extends React.Component<Props, State> {
               justifyContent: "space-between",
             }}
           >
-            {this.pluginWeightConfigs()}
+            {this._renderPluginWeightConfigs()}
           </div>
         )}
       </React.Fragment>
     );
   }
 
-  pluginWeightConfigs() {
+  _renderPluginWeightConfigs() {
     return this.props.adapters
       .adapters()
       .filter((x) => x.name() !== FALLBACK_NAME)
       .map((adapter) => {
         const onChange = (weightedTypes) => {
-          this.state.pluginNameToWeights.set(adapter.name(), weightedTypes);
-          this.fire();
+          const newWeightedTypes = {
+            nodes: MapUtil.copy(this.props.weightedTypes.nodes),
+            edges: MapUtil.copy(this.props.weightedTypes.edges),
           };
+          for (const [key, val] of weightedTypes.nodes.entries()) {
+            newWeightedTypes.nodes.set(key, val);
+          }
+          for (const [key, val] of weightedTypes.edges.entries()) {
+            newWeightedTypes.edges.set(key, val);
+          }
+          this.props.onChange(newWeightedTypes);
+        };
+        const pluginScopedWeightedTypes = {
+          nodes: new Map(),
+          edges: new Map(),
+        };
+        for (const {prefix} of adapter.nodeTypes()) {
+          pluginScopedWeightedTypes.nodes.set(
+            prefix,
+            NullUtil.get(this.props.weightedTypes.nodes.get(prefix))
+          );
+        }
+        for (const {prefix} of adapter.edgeTypes()) {
+          pluginScopedWeightedTypes.edges.set(
+            prefix,
+            NullUtil.get(this.props.weightedTypes.edges.get(prefix))
+          );
+        }
         return (
           <PluginWeightConfig
             key={adapter.name()}
             adapter={adapter}
             onChange={onChange}
+            weightedTypes={pluginScopedWeightedTypes}
           />
         );
       });
   }
-
-  fire() {
-    const weights = combineWeights(
-      this.props.adapters
-        .adapters()
-        .map((adapter: StaticPluginAdapter) =>
-          NullUtil.orElse(
-            this.state.pluginNameToWeights.get(adapter.name()),
-            defaultWeightsForAdapter(adapter)
-          )
-        )
-    );
-    this.props.onChange(weights);
-  }
 }

src/app/credExplorer/WeightConfig.test.js

@@ -0,0 +1,99 @@
+// @flow
+
+import React from "react";
+import {shallow} from "enzyme";
+import {PluginWeightConfig} from "./weights/PluginWeightConfig";
+import {
+  staticAdapterSet,
+  inserterNodeType,
+  FactorioStaticAdapter,
+} from "../adapters/demoAdapters";
+import {FALLBACK_NAME} from "../adapters/fallbackAdapter";
+import {
+  defaultWeightsForAdapterSet,
+  defaultWeightsForAdapter,
+} from "./weights/weights";
+import {WeightConfig} from "./WeightConfig";
+
+require("../testUtil").configureEnzyme();
+
+describe("app/credExplorer/WeightConfig", () => {
+  describe("WeightConfig", () => {
+    function example() {
+      const onChange = jest.fn();
+      const adapters = staticAdapterSet();
+      const types = defaultWeightsForAdapterSet(adapters);
+      types.nodes.set(inserterNodeType.prefix, {
+        weight: 707,
+        type: inserterNodeType,
+      });
+      const el = shallow(
+        <WeightConfig
+          adapters={adapters}
+          weightedTypes={types}
+          onChange={onChange}
+        />
+      );
+      // TODO(@decentralion): WeightConfig stops expanding itself
+      el.setState({expanded: true});
+      return {el, adapters, types, onChange};
+    }
+    it("creates a PluginWeightConfig for every non-fallback adapter", () => {
+      const {el, adapters} = example();
+      const pwcs = el.find(PluginWeightConfig);
+      expect(pwcs).toHaveLength(adapters.adapters().length - 1);
+      for (const adapter of adapters.adapters()) {
+        if (adapter.name() === FALLBACK_NAME) {
+          continue;
+        }
+        const pwc = pwcs.findWhere(
+          (x) => x.props().adapter.name() === adapter.name()
+        );
+        expect(pwc).toHaveLength(1);
+      }
+    });
+    it("sets the PluginWeightConfig weights properly", () => {
+      const {el} = example();
+      const pwc = el
+        .find(PluginWeightConfig)
+        .findWhere(
+          (x) => x.props().adapter.name() === new FactorioStaticAdapter().name()
+        );
+      expect(pwc).toHaveLength(1);
+      const expectedTypes = defaultWeightsForAdapter(
+        new FactorioStaticAdapter()
+      );
+      expectedTypes.nodes.set(inserterNodeType.prefix, {
+        weight: 707,
+        type: inserterNodeType,
+      });
+      expect(pwc.props().weightedTypes).toEqual(expectedTypes);
+    });
+    it("sets the PluginWeightConfig onChange properly", () => {
+      const newFactorioWeights = defaultWeightsForAdapter(
+        new FactorioStaticAdapter()
+      );
+      newFactorioWeights.nodes.set(inserterNodeType.prefix, {
+        weight: 955,
+        type: inserterNodeType,
+      });
+      const expectedFullWeights = defaultWeightsForAdapterSet(
+        staticAdapterSet()
+      );
+      expectedFullWeights.nodes.set(inserterNodeType.prefix, {
+        weight: 955,
+        type: inserterNodeType,
+      });
+
+      const {el, onChange} = example();
+      const factorioConfig = el
+        .find(PluginWeightConfig)
+        .findWhere(
+          (x) => x.props().adapter.name() === new FactorioStaticAdapter().name()
+        );
+      factorioConfig.props().onChange(newFactorioWeights);
+      expect(onChange).toHaveBeenCalledTimes(1);
+      expect(onChange).toHaveBeenCalledWith(expectedFullWeights);
+    });
+  });
+});

src/app/credExplorer/weights/PluginWeightConfig.js

@@ -1,6 +1,7 @@
 // @flow
 
 import React from "react";
+import deepEqual from "lodash.isequal";
 import * as MapUtil from "../../../util/map";
 import {NodeTypeConfig} from "./NodeTypeConfig";
 import {EdgeTypeConfig} from "./EdgeTypeConfig";
@@ -10,45 +11,27 @@ import {
   type WeightedTypes,
   type WeightedEdgeType,
   type WeightedNodeType,
-  defaultWeightsForAdapter,
 } from "./weights";
 
 export type Props = {|
   +adapter: StaticPluginAdapter,
   +onChange: (WeightedTypes) => void,
-|};
-export type State = {|
-  weights: WeightedTypes,
+  +weightedTypes: WeightedTypes,
 |};
 
-export class PluginWeightConfig extends React.Component<Props, State> {
+export class PluginWeightConfig extends React.Component<Props> {
   constructor(props: Props) {
     super(props);
-    const weights = defaultWeightsForAdapter(this.props.adapter);
-    this.state = {weights};
-  }
-
-  fire() {
-    this.props.onChange(this.state.weights);
-  }
-
-  componentDidMount() {
-    this.fire();
   }
 
   _renderNodeWeightControls() {
-    return Array.from(this.state.weights.nodes.values()).map(
+    return Array.from(this.props.weightedTypes.nodes.values()).map(
       (wnt: WeightedNodeType) => {
         const onChange = (newType: WeightedNodeType) => {
-          this.setState(
-            (state) => {
-              const newNodes = MapUtil.copy(state.weights.nodes);
+          const newNodes = MapUtil.copy(this.props.weightedTypes.nodes);
           newNodes.set(newType.type.prefix, newType);
-              const weights = {...state.weights, nodes: newNodes};
-              return {weights};
-            },
-            () => this.fire()
-          );
+          const weights = {...this.props.weightedTypes, nodes: newNodes};
+          this.props.onChange(weights);
         };
         return (
           <NodeTypeConfig
@@ -62,18 +45,13 @@ export class PluginWeightConfig extends React.Component<Props, State> {
   }
 
   _renderEdgeWeightControls() {
-    return Array.from(this.state.weights.edges.values()).map(
+    return Array.from(this.props.weightedTypes.edges.values()).map(
       (wnt: WeightedEdgeType) => {
         const onChange = (newType: WeightedEdgeType) => {
-          this.setState(
-            (state) => {
-              const newEdges = MapUtil.copy(state.weights.edges);
+          const newEdges = MapUtil.copy(this.props.weightedTypes.edges);
           newEdges.set(newType.type.prefix, newType);
-              const weights = {...state.weights, edges: newEdges};
-              return {weights};
-            },
-            () => this.fire()
-          );
+          const weights = {...this.props.weightedTypes, edges: newEdges};
+          this.props.onChange(weights);
         };
         return (
           <EdgeTypeConfig
@@ -86,7 +64,25 @@ export class PluginWeightConfig extends React.Component<Props, State> {
     );
   }
 
+  _validateWeightedTypesWithAdapter() {
+    const expectedNodePrefixes = new Set(
+      this.props.adapter.nodeTypes().map((x) => x.prefix)
+    );
+    const actualNodePrefixes = new Set(this.props.weightedTypes.nodes.keys());
+    if (!deepEqual(expectedNodePrefixes, actualNodePrefixes)) {
+      throw new Error("weightedTypes has wrong node prefixes for adapter");
+    }
+    const expectedEdgePrefixes = new Set(
+      this.props.adapter.edgeTypes().map((x) => x.prefix)
+    );
+    const actualEdgePrefixes = new Set(this.props.weightedTypes.edges.keys());
+    if (!deepEqual(expectedEdgePrefixes, actualEdgePrefixes)) {
+      throw new Error("weightedTypes has wrong edge prefixes for adapter");
+    }
+  }
+
   render() {
+    this._validateWeightedTypesWithAdapter();
     return (
       <div>
         <h3>{this.props.adapter.name()}</h3>

src/app/credExplorer/weights/PluginWeightConfig.test.js

@@ -3,32 +3,41 @@
 import React from "react";
 import {shallow} from "enzyme";
 import {PluginWeightConfig} from "./PluginWeightConfig";
-import {FactorioStaticAdapter} from "../../adapters/demoAdapters";
+import {
+  FactorioStaticAdapter,
+  inserterNodeType,
+  assemblesEdgeType,
+} from "../../adapters/demoAdapters";
+import {
+  fallbackNodeType,
+  fallbackEdgeType,
+} from "../../adapters/fallbackAdapter";
 import {NodeTypeConfig} from "./NodeTypeConfig";
 import {EdgeTypeConfig} from "./EdgeTypeConfig";
 import {
   defaultWeightsForAdapter,
   defaultWeightedNodeType,
   defaultWeightedEdgeType,
+  type WeightedTypes,
 } from "./weights";
 
 require("../../testUtil").configureEnzyme();
 
-describe("src/app/credExplorer/weights/PluginWeightConfig", () => {
+describe("app/credExplorer/weights/PluginWeightConfig", () => {
   describe("PluginWeightConfig", () => {
     function example() {
       const onChange = jest.fn();
       const adapter = new FactorioStaticAdapter();
+      const types = defaultWeightsForAdapter(adapter);
       const el = shallow(
-        <PluginWeightConfig adapter={adapter} onChange={onChange} />
+        <PluginWeightConfig
+          weightedTypes={types}
+          adapter={adapter}
+          onChange={onChange}
+        />
       );
       return {el, onChange, adapter};
     }
-    it("fires plugin's default weights on mount", () => {
-      const {onChange, adapter} = example();
-      const expected = defaultWeightsForAdapter(adapter);
-      expect(onChange).toHaveBeenCalledWith(expected);
-    });
     it("renders a NodeTypeConfig for each node type", () => {
       const {el, adapter} = example();
       const ntc = el.find(NodeTypeConfig);
@@ -61,8 +70,8 @@ describe("src/app/credExplorer/weights/PluginWeightConfig", () => {
         ),
       };
       ntc.props().onChange(newWeightedType);
-      expect(onChange).toHaveBeenCalledTimes(2);
-      expect(onChange.mock.calls[1][0]).toEqual(expected);
+      expect(onChange).toHaveBeenCalledTimes(1);
+      expect(onChange).toHaveBeenCalledWith(expected);
     });
     it("EdgeTypeConfig onChange wired properly", () => {
       const {el, adapter, onChange} = example();
@@ -77,8 +86,48 @@ describe("src/app/credExplorer/weights/PluginWeightConfig", () => {
         edges: new Map(newEdges.map((x) => [x.type.prefix, x])),
       };
       ntc.props().onChange(newWeightedType);
-      expect(onChange).toHaveBeenCalledTimes(2);
-      expect(onChange.mock.calls[1][0]).toEqual(expected);
+      expect(onChange).toHaveBeenCalledTimes(1);
+      expect(onChange).toHaveBeenCalledWith(expected);
+    });
+    describe("errors if", () => {
+      function checkError(weightedTypes: WeightedTypes) {
+        expect(() =>
+          shallow(
+            <PluginWeightConfig
+              adapter={new FactorioStaticAdapter()}
+              weightedTypes={weightedTypes}
+              onChange={jest.fn()}
+            />
+          )
+        ).toThrowError("prefixes for adapter");
+      }
+      it("missing edge prefix in weighted types", () => {
+        const badTypes = defaultWeightsForAdapter(new FactorioStaticAdapter());
+        badTypes.nodes.delete(inserterNodeType.prefix);
+        checkError(badTypes);
+      });
+      it("missing node prefix in weighted types", () => {
+        const badTypes = defaultWeightsForAdapter(new FactorioStaticAdapter());
+        badTypes.edges.delete(assemblesEdgeType.prefix);
+        checkError(badTypes);
+      });
+      it("extra node prefix in weighted types", () => {
+        const badTypes = defaultWeightsForAdapter(new FactorioStaticAdapter());
+        badTypes.nodes.set(fallbackNodeType.prefix, {
+          weight: 5,
+          type: fallbackNodeType,
+        });
+        checkError(badTypes);
+      });
+      it("extra edge prefix in weighted types", () => {
+        const badTypes = defaultWeightsForAdapter(new FactorioStaticAdapter());
+        badTypes.edges.set(fallbackEdgeType.prefix, {
+          forwardWeight: 5,
+          backwardWeight: 3,
+          type: fallbackEdgeType,
+        });
+        checkError(badTypes);
+      });
     });
   });
 });