Skip to content

Commit

Permalink
Type the correct prop passed to InteractiveGraphEditor (#1599)
Browse files Browse the repository at this point in the history 
Previously, `correct` was typed as `any` to suppress an error. TypeScript
complained that the InteractiveGraph component might pass the wrong subtype of
`PerseusGraphType` to its `onChange` callback, causing us to create a
nonsensical chimera graph when we do stuff like `correct = {...correct,
...newGraph};`

This PR adds runtime checks (mostly using `tiny-invariant`) to ensure that
graphs have the type we expect.

In doing this refactoring, I discovered a second problem: the
InteractiveGraphEditor sometimes sets the `coords` of the graph to `null`,
which wasn't allowed by `PerseusGraphType` - only `undefined` was allowed.
This PR fixes that by updating `PerseusGraphType` to allow null coords. The
interactive graph code can (thankfully) already handle null values just fine.

Issue: none

## Test plan:
Edit an interactive graph in the exercise editor.

Author: benchristel

Reviewers: nishasy, jeremywiebe, benchristel, mark-fitzgerald

Required Reviewers:

Approved By: nishasy, jeremywiebe

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1599
  • Loading branch information
@benchristel
benchristel authored Sep 10, 2024
1 parent 9718fcc commit 71715af
Show file tree
Hide file tree
Showing 5 changed files with 158 additions and 43 deletions.
6 changes: 6 additions & 0 deletions .changeset/rude-mangos-boil.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/perseus": patch
"@khanacademy/perseus-editor": patch
---

Internal: improve type safety of interactive graph editor
174 changes: 140 additions & 34 deletions packages/perseus-editor/src/widgets/interactive-graph-editor/interactive-graph-editor.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import {vector as kvector} from "@khanacademy/kmath";
import {
components,
interactiveSizes,
InteractiveGraphWidget,
interactiveSizes,
SizingUtils,
Util,
} from "@khanacademy/perseus";
Expand All @@ -11,8 +11,10 @@ import {OptionItem, SingleSelect} from "@khanacademy/wonder-blocks-dropdown";
import {Checkbox} from "@khanacademy/wonder-blocks-form";
import {spacing} from "@khanacademy/wonder-blocks-tokens";
import {LabelSmall} from "@khanacademy/wonder-blocks-typography";
import {UnreachableCaseError} from "@khanacademy/wonder-stuff-core";
import {StyleSheet} from "aphrodite";
import * as React from "react";
import invariant from "tiny-invariant";
import _ from "underscore";

import LabeledRow from "../../components/graph-locked-figures/labeled-row";
Expand All @@ -28,10 +30,11 @@ import {shouldShowStartCoordsUI} from "../../components/util";
import {parsePointCount} from "../../util/points";

import type {
PerseusImageBackground,
PerseusInteractiveGraphWidgetOptions,
APIOptionsWithDefaults,
LockedFigure,
PerseusImageBackground,
PerseusInteractiveGraphWidgetOptions,
PerseusGraphType,
} from "@khanacademy/perseus";
import type {PropsFor} from "@khanacademy/wonder-blocks-core";

Expand All @@ -56,6 +59,8 @@ const POLYGON_SIDES = _.map(_.range(3, 13), function (value) {
});

type Range = [min: number, max: number];
type PerseusGraphTypePolygon = Extract<PerseusGraphType, {type: "polygon"}>;
type PerseusGraphTypeAngle = Extract<PerseusGraphType, {type: "angle"}>;

export type Props = {
apiOptions: APIOptionsWithDefaults;
Expand Down Expand Up @@ -120,7 +125,8 @@ export type Props = {
* on the state of the interactive graph previewed at the bottom of the
* editor page.
*/
correct: any; // TODO(jeremy)
// TODO(LEMS-2344): make the type of `correct` more specific
correct: PerseusGraphType;
/**
* The locked figures to display in the graph area.
* Locked figures are graph elements (points, lines, line segmeents,
Expand Down Expand Up @@ -176,14 +182,6 @@ class InteractiveGraphEditor extends React.Component<Props> {
},
};

changeMatchType = (newValue) => {
const correct = {
...this.props.correct,
match: newValue,
};
this.props.onChange({correct: correct});
};

changeStartCoords = (coords) => {
if (!this.props.graph?.type) {
return;
Expand Down Expand Up @@ -306,17 +304,16 @@ class InteractiveGraphEditor extends React.Component<Props> {
fullGraphAriaLabel: this.props.fullGraphAriaLabel,
fullGraphAriaDescription: this.props.fullGraphAriaDescription,
trackInteraction: function () {},
onChange: (newProps: InteractiveGraphProps) => {
onChange: ({graph: newGraph}: InteractiveGraphProps) => {
let correct = this.props.correct;
// @ts-expect-error - TS2532 - Object is possibly 'undefined'.
if (correct.type === newProps.graph.type) {
correct = {
...correct,
...newProps.graph,
};
// TODO(benchristel): can we improve the type of onChange
// so this invariant isn't necessary?
invariant(newGraph != null);
if (correct.type === newGraph.type) {
correct = mergeGraphs(correct, newGraph);
} else {
// Clear options from previous graph
correct = newProps.graph;
correct = newGraph;
}
this.props.onChange({
correct: correct,
Expand Down Expand Up @@ -411,19 +408,27 @@ class InteractiveGraphEditor extends React.Component<Props> {
}
placeholder=""
onChange={(newValue) => {
const graph = {
...this.props.correct,
invariant(
this.props.graph?.type === "polygon",
);
const updates = {
numSides: parsePointCount(newValue),
coords: null,
// reset the snap for UNLIMITED, which
// only supports "grid"
// From: D6578
snapTo: "grid",
};
} as const;

this.props.onChange({
correct: graph,
graph: graph,
correct: {
...this.props.correct,
...updates,
},
graph: {
...this.props.graph,
...updates,
},
});
}}
style={styles.singleSelectShort}
Expand All @@ -446,15 +451,29 @@ class InteractiveGraphEditor extends React.Component<Props> {
// Never uses placeholder, always has value
placeholder=""
onChange={(newValue) => {
const graph = {
...this.props.correct,
snapTo: newValue,
invariant(
this.props.correct.type === "polygon",
`Expected correct answer type to be polygon, but got ${this.props.correct.type}`,
);
invariant(
this.props.graph?.type === "polygon",
`Expected graph type to be polygon, but got ${this.props.graph?.type}`,
);

const updates = {
snapTo: newValue as PerseusGraphTypePolygon["snapTo"],
coords: null,
};
} as const;

this.props.onChange({
correct: graph,
graph: graph,
correct: {
...this.props.correct,
...updates,
},
graph: {
...this.props.graph,
...updates,
},
});
}}
style={styles.singleSelectShort}
Expand Down Expand Up @@ -500,6 +519,11 @@ class InteractiveGraphEditor extends React.Component<Props> {
}
onChange={() => {
if (this.props.graph?.type === "polygon") {
invariant(
this.props.correct.type ===
"polygon",
`Expected graph type to be polygon, but got ${this.props.correct.type}`,
);
this.props.onChange({
correct: {
...this.props.correct,
Expand Down Expand Up @@ -531,7 +555,10 @@ class InteractiveGraphEditor extends React.Component<Props> {
!!this.props.correct?.showSides
}
onChange={() => {
if (this.props.graph?.type === "polygon") {
if (
this.props.graph?.type === "polygon" &&
this.props.correct.type === "polygon"
) {
this.props.onChange({
correct: {
...this.props.correct,
Expand Down Expand Up @@ -605,7 +632,24 @@ class InteractiveGraphEditor extends React.Component<Props> {
<LabeledRow label="Student answer must">
<SingleSelect
selectedValue={this.props.correct.match || "exact"}
onChange={this.changeMatchType}
onChange={(newValue) => {
invariant(
this.props.correct.type === "polygon",
`Expected graph type to be polygon, but got ${this.props.correct.type}`,
);
const correct = {
...this.props.correct,
// TODO(benchristel): this cast is necessary
// because "exact" is not actually a valid
// value for `match`; a value of undefined
// means exact matching. The code happens
// to work because "exact" falls through
// to the correct else branch in
// InteractiveGraph.validate()
match: newValue as PerseusGraphTypePolygon["match"],
};
this.props.onChange({correct});
}}
// Never uses placeholder, always has value
placeholder=""
style={styles.singleSelectShort}
Expand Down Expand Up @@ -668,7 +712,21 @@ class InteractiveGraphEditor extends React.Component<Props> {
<LabeledRow label="Student answer must">
<SingleSelect
selectedValue={this.props.correct.match || "exact"}
onChange={this.changeMatchType}
onChange={(newValue) => {
this.props.onChange({
correct: {
...this.props.correct,
// TODO(benchristel): this cast is necessary
// because "exact" is not actually a valid
// value for `match`; a value of undefined
// means exact matching. The code happens
// to work because "exact" falls through
// to the correct else branch in
// InteractiveGraph.validate()
match: newValue as PerseusGraphTypeAngle["match"],
},
});
}}
// Never uses placeholder, always has value
placeholder=""
style={styles.singleSelectShort}
Expand Down Expand Up @@ -718,6 +776,54 @@ class InteractiveGraphEditor extends React.Component<Props> {
}
}

// Merges two graphs that have the same `type`. Properties defined in `b`
// overwrite properties of the same name in `a`. Throws an exception if the
// types are different or not recognized.
function mergeGraphs(
a: PerseusGraphType,
b: PerseusGraphType,
): PerseusGraphType {
if (a.type !== b.type) {
throw new Error(
`Cannot merge graphs with different types (${a.type} and ${b.type})`,
);
}
switch (a.type) {
case "angle":
invariant(b.type === "angle");
return {...a, ...b};
case "circle":
invariant(b.type === "circle");
return {...a, ...b};
case "linear":
invariant(b.type === "linear");
return {...a, ...b};
case "linear-system":
invariant(b.type === "linear-system");
return {...a, ...b};
case "point":
invariant(b.type === "point");
return {...a, ...b};
case "polygon":
invariant(b.type === "polygon");
return {...a, ...b};
case "quadratic":
invariant(b.type === "quadratic");
return {...a, ...b};
case "ray":
invariant(b.type === "ray");
return {...a, ...b};
case "segment":
invariant(b.type === "segment");
return {...a, ...b};
case "sinusoid":
invariant(b.type === "sinusoid");
return {...a, ...b};
default:
throw new UnreachableCaseError(a);
}
}

const styles = StyleSheet.create({
singleSelectShort: {
// Non-standard spacing, but it's the smallest we can go
Expand Down
19 changes: 10 additions & 9 deletions packages/perseus/src/perseus-types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,7 @@ export type PerseusInteractiveGraphWidgetOptions = {
// The type of graph
graph: PerseusGraphType;
// The correct kind of graph, if being used to select function type
// TODO(LEMS-2344): make the type of `correct` more specific
correct: PerseusGraphType;
// Shapes (points, chords, etc) displayed on the graph that cannot
// be moved by the user.
Expand Down Expand Up @@ -816,7 +817,7 @@ export type PerseusGraphTypeAngle = {
// How to match the answer. If missing, defaults to exact matching.
match?: "congruent";
// must have 3 coords - ie [Coord, Coord, Coord]
coords?: [Coord, Coord, Coord];
coords?: [Coord, Coord, Coord] | null;
// The initial coordinates the graph renders with.
startCoords?: [Coord, Coord, Coord];
};
Expand All @@ -835,15 +836,15 @@ export type PerseusGraphTypeCircle = {
export type PerseusGraphTypeLinear = {
type: "linear";
// expects 2 coords
coords?: CollinearTuple;
coords?: CollinearTuple | null;
// The initial coordinates the graph renders with.
startCoords?: CollinearTuple;
} & PerseusGraphTypeCommon;

export type PerseusGraphTypeLinearSystem = {
type: "linear-system";
// expects 2 sets of 2 coords
coords?: CollinearTuple[];
coords?: CollinearTuple[] | null;
// The initial coordinates the graph renders with.
startCoords?: CollinearTuple[];
} & PerseusGraphTypeCommon;
Expand All @@ -852,7 +853,7 @@ export type PerseusGraphTypePoint = {
type: "point";
// The number of points if a "point" type. default: 1. "unlimited" if no limit
numPoints?: number | "unlimited";
coords?: ReadonlyArray<Coord>;
coords?: ReadonlyArray<Coord> | null;
// The initial coordinates the graph renders with.
startCoords?: ReadonlyArray<Coord>;
} & PerseusGraphTypeCommon;
Expand All @@ -869,15 +870,15 @@ export type PerseusGraphTypePolygon = {
snapTo?: "grid" | "angles" | "sides";
// How to match the answer. If missing, defaults to exact matching.
match?: "similar" | "congruent" | "approx";
coords?: ReadonlyArray<Coord>;
coords?: ReadonlyArray<Coord> | null;
// The initial coordinates the graph renders with.
startCoords?: ReadonlyArray<Coord>;
} & PerseusGraphTypeCommon;

export type PerseusGraphTypeQuadratic = {
type: "quadratic";
// expects a list of 3 coords
coords?: [Coord, Coord, Coord];
coords?: [Coord, Coord, Coord] | null;
// The initial coordinates the graph renders with.
startCoords?: [Coord, Coord, Coord];
} & PerseusGraphTypeCommon;
Expand All @@ -887,23 +888,23 @@ export type PerseusGraphTypeSegment = {
// The number of segments if a "segment" type. default: 1. Max: 6
numSegments?: number;
// Expects a list of Coord tuples. Length should match the `numSegments` value.
coords?: CollinearTuple[];
coords?: CollinearTuple[] | null;
// The initial coordinates the graph renders with.
startCoords?: CollinearTuple[];
} & PerseusGraphTypeCommon;

export type PerseusGraphTypeSinusoid = {
type: "sinusoid";
// Expects a list of 2 Coords
coords?: ReadonlyArray<Coord>;
coords?: ReadonlyArray<Coord> | null;
// The initial coordinates the graph renders with.
startCoords?: ReadonlyArray<Coord>;
} & PerseusGraphTypeCommon;

export type PerseusGraphTypeRay = {
type: "ray";
// Expects a list of 2 Coords
coords?: CollinearTuple;
coords?: CollinearTuple | null;
// The initial coordinates the graph renders with.
startCoords?: CollinearTuple;
} & PerseusGraphTypeCommon;
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/interactive-graph.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ const makeInvalidTypeError = (

type RenderProps = PerseusInteractiveGraphWidgetOptions; // There's no transform function in exports
export type Rubric = {
// TODO(LEMS-2344): make the type of `correct` more specific
correct: PerseusGraphType;
graph: PerseusGraphType;
};
Expand Down
Loading

0 comments on commit 71715af

Please sign in to comment.