From 265a9310486e5c1524af9b502619db9de2f7c01d Mon Sep 17 00:00:00 2001 From: Ben Christel Date: Mon, 6 Jan 2025 16:02:53 -0800 Subject: [PATCH] Redesign discriminated union type parser (#2068) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the `discriminatedUnion` parser required you to specify the discriminant via an object parser, which was confusing and verbose. Now you only have to specify the discriminant key once, and provide the values for each branch. Issue: LEMS-2582 ## Test plan: `yarn test` Author: benchristel Reviewers: jeremywiebe, benchristel, anakaren-rojas, catandthemachines, nishasy Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: https://github.com/Khan/perseus/pull/2068 --- .changeset/many-cats-run.md | 5 + .../discriminated-union.test.ts | 129 +++++++++++------- .../discriminated-union.ts | 98 ++++++++----- .../discriminated-union.typetest.ts | 65 +++++++++ .../perseus-parsers/grapher-widget.ts | 41 +++--- .../perseus-parsers/interaction-widget.ts | 29 ++-- 6 files changed, 247 insertions(+), 120 deletions(-) create mode 100644 .changeset/many-cats-run.md create mode 100644 packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.typetest.ts diff --git a/.changeset/many-cats-run.md b/.changeset/many-cats-run.md new file mode 100644 index 0000000000..9c376f27da --- /dev/null +++ b/.changeset/many-cats-run.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus": patch +--- + +Internal: Redesign discriminated union type parser to have a simpler and more intuitive interface. diff --git a/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.test.ts b/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.test.ts index 65ddab991d..635de19fff 100644 --- a/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.test.ts +++ b/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.test.ts @@ -2,83 +2,118 @@ import {parse} from "../parse"; import {failure, success} from "../result"; import {constant} from "./constant"; -import {discriminatedUnion} from "./discriminated-union"; +import {discriminatedUnionOn} from "./discriminated-union"; import {number} from "./number"; import {object} from "./object"; -describe("a discriminatedUnion with one variant", () => { - const unionParser = discriminatedUnion( - object({type: constant("ok")}), - object({type: constant("ok"), value: number}), - ).parser; - - it("parses a valid value", () => { - const input = {type: "ok", value: 3}; +describe("a discriminatedUnion with no variants", () => { + const parseUnion = discriminatedUnionOn("shape").parser; - expect(parse(input, unionParser)).toEqual(success(input)); + it("fails appropriately given a non-object", () => { + expect(parse(true, parseUnion)).toEqual( + failure("At (root) -- expected object, but got true"), + ); }); - it("rejects a value with the wrong `type`", () => { - const input = {type: "bad", value: 3}; - - expect(parse(input, unionParser)).toEqual( - failure(`At (root).type -- expected "ok", but got "bad"`), + it("fails appropriately given an object without the discriminant key", () => { + expect(parse({}, parseUnion)).toEqual( + failure( + "At (root).shape -- expected a valid value, but got undefined", + ), ); }); - it("rejects a value with a valid type but wrong fields", () => { - const input = {type: "ok", value: "foobar"}; - - expect(parse(input, unionParser)).toEqual( - failure(`At (root).value -- expected number, but got "foobar"`), + it("fails appropriately given an object with the discriminant key", () => { + expect(parse({shape: "squarle"}, parseUnion)).toEqual( + failure( + `At (root).shape -- expected a valid value, but got "squarle"`, + ), ); }); }); -describe("a discriminatedUnion with two variants", () => { - const unionParser = discriminatedUnion( - object({type: constant("rectangle")}), - object({type: constant("rectangle"), width: number}), - ).or( - object({type: constant("circle")}), - object({type: constant("circle"), radius: number}), +describe("a discriminatedUnion with one variant", () => { + const parseCircle = object({shape: constant("circle"), radius: number}); + const parseUnion = discriminatedUnionOn("shape").withBranch( + "circle", + parseCircle, ).parser; - it("parses a valid rectangle", () => { - const input = {type: "rectangle", width: 42}; - - expect(parse(input, unionParser)).toEqual(success(input)); + it("fails appropriately given a non-object", () => { + expect(parse(true, parseUnion)).toEqual( + failure("At (root) -- expected object, but got true"), + ); }); - it("rejects a rectangle with no width", () => { - const input = {type: "rectangle", radius: 99}; - - expect(parse(input, unionParser)).toEqual( - failure(`At (root).width -- expected number, but got undefined`), + it("fails appropriately given an object without the discriminant key", () => { + expect(parse({}, parseUnion)).toEqual( + failure( + "At (root).shape -- expected a valid value, but got undefined", + ), ); }); - it("parses a valid circle", () => { - const input = {type: "circle", radius: 7}; + it("fails appropriately given an object with an invalid discriminant", () => { + expect(parse({shape: "squarle"}, parseUnion)).toEqual( + failure( + `At (root).shape -- expected a valid value, but got "squarle"`, + ), + ); + }); - expect(parse(input, unionParser)).toEqual(success(input)); + it("succeeds given a valid object", () => { + const input = {shape: "circle", radius: 3}; + expect(parse(input, parseUnion)).toEqual(success(input)); }); +}); - it("rejects a circle with no radius", () => { - const input = {type: "circle", width: 99}; +describe("a discriminatedUnion with two variants", () => { + const parseCircle = object({shape: constant("circle"), radius: number}); + const parseRectangle = object({ + shape: constant("rectangle"), + width: number, + height: number, + }); + const parseUnion = discriminatedUnionOn("shape") + .withBranch("circle", parseCircle) + .withBranch("rectangle", parseRectangle).parser; - expect(parse(input, unionParser)).toEqual( - failure(`At (root).radius -- expected number, but got undefined`), + it("fails appropriately given a non-object", () => { + expect(parse(true, parseUnion)).toEqual( + failure("At (root) -- expected object, but got true"), ); }); - it("rejects a value with an unrecognized `type`", () => { - const input = {type: "triangle", width: -1, radius: 99}; + it("fails appropriately given an object without the discriminant key", () => { + expect(parse({}, parseUnion)).toEqual( + failure( + "At (root).shape -- expected a valid value, but got undefined", + ), + ); + }); - expect(parse(input, unionParser)).toEqual( + it("fails appropriately given an object with an invalid discriminant", () => { + expect(parse({shape: "squarle"}, parseUnion)).toEqual( failure( - `At (root).type -- expected "rectangle", but got "triangle"`, + `At (root).shape -- expected a valid value, but got "squarle"`, ), ); }); + + it("successfully parses the first branch", () => { + const input = {shape: "circle", radius: 3}; + expect(parse(input, parseUnion)).toEqual(success(input)); + }); + + it("successfully parses the second branch", () => { + const input = {shape: "rectangle", width: 2, height: 4}; + expect(parse(input, parseUnion)).toEqual(success(input)); + }); + + it("doesn't try other branches after finding one that matches the discriminant key", () => { + const input = {shape: "circle", width: 2, height: 4}; + expect(parse(input, parseUnion)).toEqual( + failure(`At (root).radius -- expected number, but got undefined`), + ); + }); }); diff --git a/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.ts b/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.ts index 35e30cecc7..476bac6209 100644 --- a/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.ts +++ b/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.ts @@ -1,46 +1,76 @@ -import {isSuccess} from "../result"; - -import {pipeParsers} from "./pipe-parsers"; - -import type {Parser} from "../parser-types"; - -// discriminatedUnion() should be preferred over union() when parsing a -// discriminated union type, because discriminatedUnion() produces more -// understandable failure messages. It takes the discriminant as the source of -// truth for which variant is to be parsed, and expects the other data to match -// that variant. -export function discriminatedUnion( - narrow: Parser, - parseVariant: Parser, -): DiscriminatedUnionBuilder { - return new DiscriminatedUnionBuilder( - pipeParsers(narrow).then(parseVariant).parser, - ); +import {isObject} from "./is-object"; + +import type {ParseContext, Parser} from "../parser-types"; + +type Primitive = number | string | boolean | null | undefined; + +/** + * discriminatedUnion() should be preferred over union() when parsing a + * discriminated union type, because discriminatedUnion() produces more + * understandable failure messages. It takes the discriminant as the source of + * truth for which variant is to be parsed, and expects the other data to match + * that variant. + */ +export function discriminatedUnionOn(discriminantKey: DK) { + const noMoreBranches: Parser = (raw: unknown, ctx: ParseContext) => { + if (!isObject(raw)) { + return ctx.failure("object", raw); + } + return ctx + .forSubtree(discriminantKey) + .failure("a valid value", raw[discriminantKey]); + }; + + return new DiscriminatedUnionBuilder(discriminantKey, noMoreBranches); } -class DiscriminatedUnionBuilder { - constructor(public parser: Parser) {} +class DiscriminatedUnionBuilder< + DK extends string, + Union extends {[k in DK]: Primitive}, +> { + constructor( + private discriminantKey: DK, + public parser: Parser, + ) {} + + withBranch( + discriminantValue: Primitive, + parseNewVariant: Parser, + ): DiscriminatedUnionBuilder { + const parseNewBranch = discriminatedUnionBranch( + this.discriminantKey, + discriminantValue, + parseNewVariant, + this.parser, + ); - or( - narrow: Parser, - parseVariant: Parser, - ): DiscriminatedUnionBuilder { return new DiscriminatedUnionBuilder( - either(narrow, parseVariant, this.parser), + this.discriminantKey, + parseNewBranch, ); } } -function either( - narrowToA: Parser, - parseA: Parser, - parseB: Parser, -): Parser { - return (rawValue, ctx) => { - if (isSuccess(narrowToA(rawValue, ctx))) { - return parseA(rawValue, ctx); +function discriminatedUnionBranch< + DK extends string, + DV extends Primitive, + Variant extends {[k in DK]: DV}, + Rest extends {[k in DK]: DV}, +>( + discriminantKey: DK, + discriminantValue: DV, + parseVariant: Parser, + parseOtherBranches: Parser, +): Parser { + return (raw: unknown, ctx: ParseContext) => { + if (!isObject(raw)) { + return ctx.failure("object", raw); + } + + if (raw[discriminantKey] === discriminantValue) { + return parseVariant(raw, ctx); } - return parseB(rawValue, ctx); + return parseOtherBranches(raw, ctx); }; } diff --git a/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.typetest.ts b/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.typetest.ts new file mode 100644 index 0000000000..a4e060cd7b --- /dev/null +++ b/packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.typetest.ts @@ -0,0 +1,65 @@ +import {constant} from "./constant"; +import {discriminatedUnionOn} from "./discriminated-union"; +import {number} from "./number"; +import {object} from "./object"; + +import type {Parser} from "../parser-types"; + +type Figure = + | {shape: "circle"; radius: number} + | {shape: "rectangle"; width: number; height: number} + | {shape: "square"; sideLength: number}; + +const parseCircle = object({ + shape: constant("circle"), + radius: number, +}); + +const parseRectangle = object({ + shape: constant("rectangle"), + width: number, + height: number, +}); + +const parseSquare = object({ + shape: constant("square"), + sideLength: number, +}); + +// Test: parsed result is assignable to the union type +{ + const parser = discriminatedUnionOn("shape") + .withBranch("circle", parseCircle) + .withBranch("rectangle", parseRectangle) + .withBranch("square", parseSquare).parser; + + parser satisfies Parser
; + + // Guard against implicit 'any' type + // @ts-expect-error - Type '{ shape: "circle"; radius: number; }' is not assignable to type 'string'. + parser satisfies Parser; +} + +// Test: parse result with extra branches is not assignable to the union type +{ + const parser = discriminatedUnionOn("shape") + .withBranch("circle", parseCircle) + .withBranch("rectangle", parseRectangle) + .withBranch("square", parseSquare) + .withBranch("extra", object({shape: constant("extra")})).parser; + + // @ts-expect-error - Type '{shape: "extra"}' is not assignable to type 'Figure' + parser satisfies Parser
; +} + +// Test: each variant must contain the discriminant key +{ + // @ts-expect-error - property 'shape' is missing in type '{}' + discriminatedUnionOn("shape").withBranch("circle", object({})); +} + +// Test: each variant must be an object +{ + // @ts-expect-error - Type 'number' is not assignable to type '{ shape: Primitive; }'. + discriminatedUnionOn("shape").withBranch("circle", number); +} diff --git a/packages/perseus/src/util/parse-perseus-json/perseus-parsers/grapher-widget.ts b/packages/perseus/src/util/parse-perseus-json/perseus-parsers/grapher-widget.ts index 924c12ccad..1ded6aa94b 100644 --- a/packages/perseus/src/util/parse-perseus-json/perseus-parsers/grapher-widget.ts +++ b/packages/perseus/src/util/parse-perseus-json/perseus-parsers/grapher-widget.ts @@ -11,7 +11,7 @@ import { string, union, } from "../general-purpose-parsers"; -import {discriminatedUnion} from "../general-purpose-parsers/discriminated-union"; +import {discriminatedUnionOn} from "../general-purpose-parsers/discriminated-union"; import {parseWidget} from "./widget"; @@ -36,52 +36,53 @@ export const parseGrapherWidget: Parser = parseWidget( "tangent", ), ), - correct: discriminatedUnion( - object({type: constant("absolute_value")}), - object({ - type: constant("absolute_value"), - coords: pairOfPoints, - }), - ) - .or( - object({type: constant("exponential")}), + correct: discriminatedUnionOn("type") + .withBranch( + "absolute_value", + object({ + type: constant("absolute_value"), + coords: pairOfPoints, + }), + ) + .withBranch( + "exponential", object({ type: constant("exponential"), asymptote: pairOfPoints, coords: pairOfPoints, }), ) - .or( - object({type: constant("linear")}), + .withBranch( + "linear", object({ type: constant("linear"), coords: pairOfPoints, }), ) - .or( - object({type: constant("logarithm")}), + .withBranch( + "logarithm", object({ type: constant("logarithm"), asymptote: pairOfPoints, coords: pairOfPoints, }), ) - .or( - object({type: constant("quadratic")}), + .withBranch( + "quadratic", object({ type: constant("quadratic"), coords: pairOfPoints, }), ) - .or( - object({type: constant("sinusoid")}), + .withBranch( + "sinusoid", object({ type: constant("sinusoid"), coords: pairOfPoints, }), ) - .or( - object({type: constant("tangent")}), + .withBranch( + "tangent", object({ type: constant("tangent"), coords: pairOfPoints, diff --git a/packages/perseus/src/util/parse-perseus-json/perseus-parsers/interaction-widget.ts b/packages/perseus/src/util/parse-perseus-json/perseus-parsers/interaction-widget.ts index 4bf9a3c3bb..189149ef7f 100644 --- a/packages/perseus/src/util/parse-perseus-json/perseus-parsers/interaction-widget.ts +++ b/packages/perseus/src/util/parse-perseus-json/perseus-parsers/interaction-widget.ts @@ -11,7 +11,7 @@ import { union, } from "../general-purpose-parsers"; import {defaulted} from "../general-purpose-parsers/defaulted"; -import {discriminatedUnion} from "../general-purpose-parsers/discriminated-union"; +import {discriminatedUnionOn} from "../general-purpose-parsers/discriminated-union"; import {parsePerseusImageBackground} from "./perseus-image-background"; import {parseWidget} from "./widget"; @@ -184,24 +184,15 @@ export const parseInteractionWidget: Parser = parseWidget( tickStep: pairOfNumbers, }), elements: array( - discriminatedUnion( - object({type: parseFunctionType}), - parseFunctionElement, - ) - .or(object({type: parseLabelType}), parseLabelElement) - .or(object({type: parseLineType}), parseLineElement) - .or( - object({type: parseMovableLineType}), - parseMovableLineElement, - ) - .or( - object({type: parseMovablePointType}), - parseMovablePointElement, - ) - .or(object({type: parseParametricType}), parseParametricElement) - .or(object({type: parsePointType}), parsePointElement) - .or(object({type: parseRectangleType}), parseRectangleElement) - .parser, + discriminatedUnionOn("type") + .withBranch("function", parseFunctionElement) + .withBranch("label", parseLabelElement) + .withBranch("line", parseLineElement) + .withBranch("movable-line", parseMovableLineElement) + .withBranch("movable-point", parseMovablePointElement) + .withBranch("parametric", parseParametricElement) + .withBranch("point", parsePointElement) + .withBranch("rectangle", parseRectangleElement).parser, ), }), );