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.

Sign up for GitHub

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

Jump to bottom

Type the correct prop passed to InteractiveGraphEditor #1599

Merged
merged 6 commits into from
Sep 10, 2024
Merged

Type the correct prop passed to InteractiveGraphEditor #1599

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f1ea25f
Inline changeMatchType
benchristel Sep 5, 2024
2327890
Give 'correct' prop a real type
benchristel Sep 6, 2024
f9c447a
Allow coords to be null in interactive graphs
benchristel Sep 6, 2024
05a22d1
Fix lint
benchristel Sep 6, 2024
f70b372
Add changeset
benchristel Sep 6, 2024
0003cd0
Add TODOs
benchristel Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/rude-mangos-boil.md
View file
Open in desktop
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
173 changes: 139 additions & 34 deletions packages/perseus-editor/src/widgets/interactive-graph-editor/interactive-graph-editor.tsx
View file
Open in desktop
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 @@ -27,10 +29,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 @@ -55,6 +58,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 @@ -119,7 +124,7 @@ export type Props = {
* on the state of the interactive graph previewed at the bottom of the
* editor page.
*/
correct: any; // TODO(jeremy)
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

correct: PerseusGraphType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is the right time to think about this, but do we want correct to be typed as PerseusGraphType long term?

Personally, I think it makes more sense for correct to only have the type and coords fields in it, and all the other field would be specific to graph. And in most of the places where correct is being used now, we would use graph instead. We could start with PerseusGraphType for now and make that change later?

Any thoughts on that? (@benchristel @jeremywiebe)

I could see this potentially turning into a larger discussion, so I think it could also be worth taking this offline.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be typed as what we explicitly save today. I suspect that's not the full PerseusGraphType but some stripped-down type as Nisha suggests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that would be better. I think it's a bigger effort though (I'd like to analyze our existing content to figure out what values for correct are in use). I'll add a TODO comment and create a ticket.

/**
* The locked figures to display in the graph area.
* Locked figures are graph elements (points, lines, line segmeents,
Expand Down Expand Up @@ -168,14 +173,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 @@ -294,17 +291,16 @@ class InteractiveGraphEditor extends React.Component<Props> {
showTooltips: this.props.showTooltips,
lockedFigures: this.props.lockedFigures,
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to use invariant than to do a direct check for existence? Not asking for a change, I'm just curious because I haven't really seen this invariant pattern used in other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

The invariant is a concise way of stating that we think a particular case should be impossible (in this case, newGraph should never be null) and throwing an exception if the impossible thing happens. A conditional implies that we're handling an expected case.

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 @@ -387,19 +383,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 @@ -422,15 +426,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 @@ -476,6 +494,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 @@ -507,7 +530,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 @@ -581,7 +607,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 @@ -644,7 +687,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 @@ -694,6 +751,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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this switch statement if there's already a check for a.type !== b.type above?

I feel like we should just be able to return {...a, ...b} after that? Or is this a case of TypeScript complaining if you do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, TypeScript can't figure it out.

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
18 changes: 9 additions & 9 deletions packages/perseus/src/perseus-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,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 @@ -831,15 +831,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 @@ -848,7 +848,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 @@ -865,15 +865,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 @@ -883,23 +883,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