Add and pass regression tests for PerseusItem parser #1907

benchristel · 2024-11-23T01:02:22Z

Issue: LEMS-2582

Test plan:

yarn test

khan-actions-bot · 2024-11-23T01:02:41Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/chilled-turtles-drive.md, packages/perseus/src/perseus-types.ts, packages/perseus/src/util/parse-perseus-json/parse-perseus-json.test.ts, packages/perseus/src/widgets/graded-group/graded-group.tsx, packages/perseus/src/widgets/matrix/matrix.tsx, packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/defaulted.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/categorizer-widget.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/dropdown-widget.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/expression-widget.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/graded-group-widget.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/hint.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/iframe-widget.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/images-map.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/matrix-widget.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/perseus-item.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/perseus-renderer.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/radio-widget.ts, packages/perseus/src/util/parse-perseus-json/perseus-parsers/widget.ts, packages/perseus/src/util/parse-perseus-json/regression-tests/__snapshots__/parse-perseus-json-snapshot.test.ts.snap, packages/perseus/src/util/parse-perseus-json/regression-tests/data/categorizer-missing-static.json, packages/perseus/src/util/parse-perseus-json/regression-tests/data/dropdown-missing-version.json, packages/perseus/src/util/parse-perseus-json/regression-tests/data/hint-missing-images.json, packages/perseus/src/util/parse-perseus-json/regression-tests/data/iframe-missing-static.json, packages/perseus/src/util/parse-perseus-json/regression-tests/data/item-missing-answerArea.json, packages/perseus/src/util/parse-perseus-json/regression-tests/data/matrix-missing-version.json, packages/perseus/src/util/parse-perseus-json/regression-tests/data/orderer-option-missing-images.json, packages/perseus/src/util/parse-perseus-json/regression-tests/data/question-missing-content.json, packages/perseus/src/util/parse-perseus-json/regression-tests/data/radio-choice-missing-content.json

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

github-actions · 2024-11-23T01:05:17Z

Size Change: +159 B (+0.01%)

Total Size: 1.29 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	421 kB	+159 B (+0.04%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 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`	77.9 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	698 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.68 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

jeremywiebe · 2024-11-23T01:27:23Z

packages/perseus/src/perseus-types.ts

@@ -1034,17 +1039,17 @@ export type PerseusMatcherWidgetOptions = {
 export type PerseusMatrixWidgetAnswers = ReadonlyArray<ReadonlyArray<number>>;
 export type PerseusMatrixWidgetOptions = {
    // Translatable Text; Shown before the matrix
-    prefix: string;
+    prefix?: string | undefined;


Doesn't the ? make this optional (ie. making 'undefined' a legal "value")?

Seeing both ? and undefined feels like we're saying the same thing twice.

Suggested change

prefix?: string | undefined;

prefix?: string;

You're right. I thought prefix?: string wouldn't allow the field to be explictly set to undefined, but that appears not to be the case.

packages/perseus/src/widgets/graded-group/graded-group.tsx

jeremywiebe · 2024-11-25T19:38:14Z

packages/perseus/src/util/parse-perseus-json/perseus-parsers/categorizer-widget.ts

@@ -19,7 +20,7 @@ export const parseCategorizerWidget: Parser<CategorizerWidget> = parseWidget(
        items: array(string),
        categories: array(string),
        randomizeItems: boolean,
-        static: boolean,
+        static: defaulted(boolean, () => false),
        values: array(number),
        highlightLint: optional(boolean),
        linterContext: optional(


This definitely shouldn't be part of the serialized widget options for categorizer. It is a prop passed down by the editor components during content authoring. I can imagine this is in our published content because of a bug somewhere.

No action required, just musing.

jeremywiebe · 2024-11-25T19:41:15Z

packages/perseus/src/util/parse-perseus-json/perseus-parsers/images-map.ts

+import type {PerseusImageDetail} from "../../../perseus-types";
+import type {Parser} from "../parser-types";
+
+export const parseImages: Parser<{[key: string]: PerseusImageDetail}> =


Not a required change, but the keys in these image maps should be the url to the image. Clarifying the label may be helpful for others.

Suggested change

export const parseImages: Parser<{[key: string]: PerseusImageDetail}> =

export const parseImages: Parser<{[url: string]: PerseusImageDetail}> =

jeremywiebe · 2024-11-25T19:50:00Z

packages/perseus/src/util/parse-perseus-json/perseus-parsers/perseus-renderer.ts

+        // TODO(benchristel): content is also defaulted to empty string in
+        // renderer.tsx. See if we can remove one default or the other.


I've been thinking about this as I work through this PR. Some of our Perseus widgets/components provide default props. There are places we might be able to de-duplicate these defaults to only be defaulted in our parsers. But there may be some complications on the editing side as I'm pretty sure they depend on those default props.

I've been thinking about this too. I believe the correct place to add default values is in the parsing layer. That ensures that old content always renders consistently even if we change the default props used by the widget editors. We may still need defaultProps in the widgets too, and I think that's okay.

For example, imagine we changed the default snapStep for InteractiveGraph from 0.5 to 1 in the widget's default props. If we relied on those default props to migrate legacy data that was missing snapStep, the change would propagate to a lot of existing content — some of which might not be answerable with the new default snapStep.

If, on the other hand, we defaulted snapStep to 0.5 in the parsing layer, old content would be insulated from the change to the widget editor.

By defaulting values in the parser, we can decouple the "defaults for legacy content" from the "defaults for content creators".

jeremywiebe · 2024-11-25T19:51:56Z

packages/perseus/src/util/parse-perseus-json/perseus-parsers/radio-widget.ts

@@ -19,7 +20,7 @@ export const parseRadioWidget: Parser<RadioWidget> = parseWidget(
    object({
        choices: array(
            object({
-                content: string,
+                content: defaulted(string, () => ""),


I wonder if defaulted() would be easier to use if were defined such that the second parameter could be the literal default value as an option to a function that built the default value.

Something like this:

defaulted<T|Defaulted>(Parser<T>, Defaulted | () => Defaulted)

That'd allow many of these usages to be simpler, like this:

Suggested change

content: defaulted(string, () => ""),

content: defaulted(string, ""),

The danger with that approach is that if the default value is an object or array, the same instance will be reused for every default. E.g.

defaulted(array(someParser), [])

If some other code later mutates the array, it might affect unrelated code that happens to be holding a reference to the same value. This kind of "aliasing" bug has bitten me many times in my career, and it's always a nightmare.

I believe we have a pretty good culture of avoiding unnecessary mutation, but aliasing bugs are so bad that I feel it's better to be safe than sorry.

Fair point. Let's leave it as a function then.

jeremywiebe · 2024-11-25T19:54:00Z

packages/perseus/src/util/parse-perseus-json/perseus-parsers/widget.ts

+                major: number,
+                minor: number,


Related to other notes about defaults at different parts of Perseus, I wonder if we should make these defaulted() to 0 also? Ultimately, I suspect that the entire concept of versioning may go away because we'll just have "evergreen" parsers that handle all versions of content we've ever seen (and haven't backfilled). 🤔

No action needed, just an idea.

https://github.com/Khan/perseus/blob/1e5fce35d12efdb7bcc17f0c841da95957f9a20e/packages/perseus/src/widgets.ts#L226

Those defaults would let us handle data like version: {}, right? I haven't seen any data like that. If we ever encountered it in the wild, I'd want to get a parse error rather than just silently converting it to {major: 0, minor: 0}, because it would indicate that we have a bug somewhere else.

Evergreen parsers are a possibility, but I think we might sometimes want versioning to help make complex migrations typesafe and predictable. I do wish version were just a number; that would be a lot easier to deal with in TypeScript.

Not for version: {} but for items that have no version key whatsoever.

oldWidgetInfo.version || {major: 0, minor: 0}; would only use the value after || if it is falsy, I believe.

jeremywiebe · 2024-11-25T21:39:15Z

packages/perseus/src/widgets/matrix/matrix.tsx

@@ -100,10 +102,31 @@ export function getMatrixSize(matrix: ReadonlyArray<ReadonlyArray<number>>) {
 }

 type ExternalProps = WidgetProps<
-    PerseusMatrixWidgetOptions,
+    {


Is this necessary because the widget options defines static as optional?

It's needed because of the way TypeScript deals with React default props.

The Props type argument to React.Component represents the types of the props inside the component, after defaultProps have been applied. However, we're using the *WidgetOptions types to represent the props that are passed to the widget component. It's not correct to use *WidgetOptions as the Props type argument if the component has default props.

The specific change that motivated this was that I made prefix and suffix optional on PerseusMatrixWidgetOptions. I did that because prefix and suffix aren't always present in the JSON. But inside the component, they will always be strings, because they have defaultProps.

Co-authored-by: Jeremy Wiebe <[email protected]>

github-actions · 2024-11-26T20:23:25Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1907

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

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

benchristel self-assigned this Nov 23, 2024

benchristel requested review from anakaren-rojas, catandthemachines, jeremywiebe and nishasy November 23, 2024 01:02

khan-actions-bot requested a review from a team November 23, 2024 01:02

jeremywiebe approved these changes Nov 25, 2024

View reviewed changes

benchristel and others added 12 commits November 26, 2024 12:07

Pass regression test for matrix widget

64b514d

Extract parseImages

367580b

Pass regression test for categorizer.options.static

647f501

Default radio choice content to empty string

3e2175b

Pass another regression test

3d27504

Default question content to empty string

c5a0cf8

Pass another regression test

67cf0d9

Fix lint

9df5170

Pass another regression test

8fb3143

Update packages/perseus/src/widgets/graded-group/graded-group.tsx

29a9466

Co-authored-by: Jeremy Wiebe <[email protected]>

Update tests

8cadbcf

Fix formatting

b0ed10b

benchristel force-pushed the benc/parser-regression-tests-1 branch from d6697a2 to b0ed10b Compare November 26, 2024 20:18

Add changeset

d910fe6

benchristel added 2 commits November 26, 2024 12:26

Fix formatting

adcea9c

Fix lint

7aca4b6

benchristel merged commit 3dbca96 into main Nov 26, 2024
9 checks passed

benchristel deleted the benc/parser-regression-tests-1 branch November 26, 2024 21:16

khan-actions-bot mentioned this pull request Nov 26, 2024

Version Packages #1918

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and pass regression tests for PerseusItem parser #1907

Add and pass regression tests for PerseusItem parser #1907

benchristel commented Nov 23, 2024

khan-actions-bot commented Nov 23, 2024 •

edited

Loading

github-actions bot commented Nov 23, 2024 •

edited

Loading

jeremywiebe Nov 23, 2024

benchristel Nov 25, 2024

jeremywiebe Nov 25, 2024

jeremywiebe Nov 25, 2024

jeremywiebe Nov 25, 2024

benchristel Nov 26, 2024 •

edited

Loading

jeremywiebe Nov 25, 2024

benchristel Nov 26, 2024

jeremywiebe Nov 26, 2024

jeremywiebe Nov 25, 2024

benchristel Nov 26, 2024

jeremywiebe Nov 26, 2024

jeremywiebe Nov 25, 2024

benchristel Nov 25, 2024

github-actions bot commented Nov 26, 2024 •

edited

Loading

	export const parseImages: Parser<{[key: string]: PerseusImageDetail}> =
	export const parseImages: Parser<{[url: string]: PerseusImageDetail}> =

		// TODO(benchristel): content is also defaulted to empty string in
		// renderer.tsx. See if we can remove one default or the other.

	content: defaulted(string, () => ""),
	content: defaulted(string, ""),

Add and pass regression tests for PerseusItem parser #1907

Add and pass regression tests for PerseusItem parser #1907

Conversation

benchristel commented Nov 23, 2024

Test plan:

khan-actions-bot commented Nov 23, 2024 • edited Loading

Gerald

github-actions bot commented Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benchristel Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 26, 2024 • edited Loading

npm Snapshot: Published

khan-actions-bot commented Nov 23, 2024 •

edited

Loading

github-actions bot commented Nov 23, 2024 •

edited

Loading

benchristel Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading