Type the correct prop passed to InteractiveGraphEditor

[benchristel]

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.

[khan-actions-bot]

Gerald

Required Reviewers

• @Khan/perseus for changes to .changeset/rude-mangos-boil.md, packages/perseus/src/perseus-types.ts, packages/perseus/src/widgets/interactive-graph.tsx, packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/interactive-graph-editor.tsx

---

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

[github-actions]

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (0003cd0) and published it to npm. You
can install it using the tag PR1599.

Example:

yarn add @khanacademy/perseus@PR1599

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1599

[github-actions]

Size Change: +1.07 kB (+0.12%)

Total Size: 859 kB

Filename	Size	Change
packages/perseus-editor/dist/es/index.js	276 kB	+1.8 kB (+0.65%)
packages/perseus/dist/es/index.js	417 kB	-731 B (-0.18%)

ℹ️ View Unchanged

Filename	Size
packages/kas/dist/es/index.js	38.3 kB
packages/keypad-context/dist/es/index.js	760 B
packages/kmath/dist/es/index.js	4.27 kB
packages/math-input/dist/es/index.js	78 kB
packages/math-input/dist/es/strings.js	1.79 kB
packages/perseus-core/dist/es/index.js	1.48 kB
packages/perseus-linter/dist/es/index.js	22.2 kB
packages/perseus/dist/es/strings.js	3.36 kB
packages/pure-markdown/dist/es/index.js	3.66 kB
packages/simple-markdown/dist/es/index.js	12.4 kB

_{compressed-size-action}

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.60%. Comparing base (6c4e9e1) to head (0003cd0).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1599      +/-   ##
==========================================
+ Coverage   70.46%   70.60%   +0.14%     
==========================================
  Files         546      572      +26     
  Lines      106885   111643    +4758     
  Branches     5533    11271    +5738     
==========================================
+ Hits        75319    78830    +3511     
- Misses      31450    32813    +1363     
+ Partials      116        0     -116     

see 1118 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c4e9e1...0003cd0. Read the comment docs.

[nishasy]

A couple of questions, but LGTM.

Might also be good to run this by @jeremywiebe since I think he might have more knowledge than I do about how correct is used.

[nishasy]

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

[nishasy]

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.

[jeremywiebe]

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.

[benchristel]

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.

[nishasy]

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?

[benchristel]

Yeah, TypeScript can't figure it out.

[nishasy]

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.

[benchristel]

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.

[jeremywiebe]

I can't comment on the lines I'll refer to as they weren't changed. But I wonder if we could tighten up the graph and correct types further by making the types enforce that .graph.type and .correct.type are always the same.

Something like this definition of PerseusInteractiveGraphWidgetOptions. Basically, making graph and correct keys a set of type unions where they always match:

export type PerseusInteractiveGraphWidgetOptions = {
    // Where the little black axis lines & labels (ticks) should render.
    // Also known as the tick step. default [1, 1]
    // NOTE(kevinb): perseus_data.go defines this as Array<number>
    step: [number, number];
    // Where the grid lines on the graph will render. default [1, 1]
    // NOTE(kevinb): perseus_data.go defines this as Array<number>
    gridStep: [number, number];
    // Where the graph points will lock to when they are dragged. default [0.5, 0.5]
    // NOTE(kevinb): perseus_data.go defines this as Array<number>
    snapStep: [number, number];
    // An optional image to use in the background
    backgroundImage?: PerseusImageBackground;
    /**
     * The type of markings to display on the graph.
     * - graph: shows the axes and the grid lines
     * - grid: shows only the grid lines
     * - none: shows no markings
     */
    markings: "graph" | "grid" | "none";
    // How to label the X and Y axis.  default: ["x", "y"]
    labels: ReadonlyArray<string>;
    // Whether to show the Protractor tool overlayed on top of the graph
    showProtractor: boolean;
    /**
     * Whether to show the Ruler tool overlayed on top of the graph.
     * @deprecated - no longer used by the InteractiveGraph widget. The
     * property is kept on this type to prevent its accidental reuse in future
     * features, since it may appear in production data.
     */
    showRuler?: boolean;
    // Whether to show tooltips on the graph
    showTooltips?: boolean;
    /**
     * The unit to show on the ruler.  e.g. "mm", "cm",  "m", "km", "in", "ft",
     * "yd", "mi".
     * @deprecated - no longer used by the InteractiveGraph widget. The
     * property is kept on this type to prevent its accidental reuse in future
     * features, since it may appear in production data.
     */
    rulerLabel?: string;
    /**
     * How many ticks to show on the ruler.  e.g. 1, 2, 4, 8, 10, 16. Must be
     * an integer.
     * @deprecated - no longer used by the InteractiveGraph widget. The
     * property is kept on this type to prevent its accidental reuse in future
     * features, since it may appear in production data.
     */
    rulerTicks?: number;
    // The X and Y coordinate ranges for the view of the graph.  default: [[-10, 10], [-10, 10]]
    // NOTE(kevinb): perseus_data.go defines this as Array<Array<number>>
    // TODO(kevinb): Add a transform function to interactive-graph.jsx to
    // rename `range` to `ranges` so that things are less confusing.
    range: GraphRange;
    // Shapes (points, chords, etc) displayed on the graph that cannot
    // be moved by the user.
    lockedFigures?: ReadonlyArray<LockedFigure>;
} & (
    | {
          // The type of graph
          graph: PerseusGraphTypeAngle;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypeAngle;
      }
    | {
          // The type of graph
          graph: PerseusGraphTypeCircle;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypeCircle;
      }
    | {
          // The type of graph
          graph: PerseusGraphTypeLinear;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypeLinear;
      }
    | {
          // The type of graph
          graph: PerseusGraphTypeLinearSystem;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypeLinearSystem;
      }
    | {
          // The type of graph
          graph: PerseusGraphTypePoint;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypePoint;
      }
    | {
          // The type of graph
          graph: PerseusGraphTypePolygon;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypePolygon;
      }
    | {
          // The type of graph
          graph: PerseusGraphTypeQuadratic;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypeQuadratic;
      }
    | {
          // The type of graph
          graph: PerseusGraphTypeRay;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypeRay;
      }
    | {
          // The type of graph
          graph: PerseusGraphTypeSegment;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypeSegment;
      }
    | {
          // The type of graph
          graph: PerseusGraphTypeSinusoid;
          // The correct kind of graph, if being used to select function type
          correct: PerseusGraphTypeSinusoid;
      }
);

[jeremywiebe]

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.

[benchristel]

@jeremywiebe I agree it would be cool if we could correlate the correct and graph types like that. Unfortunately, I don't think TypeScript supports that. Narrowing the type of either correct or graph does not narrow the type of the other. Example: typescript playground.