Resolve some TODO comments in the Perseus JSON parsing code #2380

benchristel · 2025-04-10T23:14:20Z

Issue: none

Test plan:

pnpm test

github-actions · 2025-04-10T23:15:40Z

Size Change: -89 B (-0.02%)

Total Size: 466 kB

Filename	Size	Change
`packages/perseus-core/dist/es/index.js`	18.5 kB	-6 B (-0.03%)
`packages/perseus-editor/dist/es/index.js`	88.4 kB	-67 B (-0.08%)
`packages/perseus/dist/es/index.js`	199 kB	-16 B (-0.01%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	20.7 kB
`packages/keypad-context/dist/es/index.js`	1 kB
`packages/kmath/dist/es/index.js`	5.98 kB
`packages/math-input/dist/es/index.js`	98.6 kB
`packages/math-input/dist/es/strings.js`	1.61 kB
`packages/perseus-linter/dist/es/index.js`	7.05 kB
`packages/perseus-score/dist/es/index.js`	9.04 kB
`packages/perseus-utils/dist/es/index.js`	403 B
`packages/perseus/dist/es/strings.js`	7.49 kB
`packages/pure-markdown/dist/es/index.js`	1.22 kB
`packages/simple-markdown/dist/es/index.js`	6.71 kB

_{compressed-size-action}

github-actions · 2025-04-10T23:16:17Z

npm Snapshot: Published

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

Example:

pnpm add @khanacademy/perseus@PR2380

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

./dev/tools/bump_perseus_version.js -t PR2380

jeremywiebe

Really happy to see how the parsers are letting us improve things. Thanks for that!

I noticed a few linter violations at the bottom of the file listing. Not sure if they're valid...

packages/perseus-core/src/parse-perseus-json/exhaustive-test-tool/index.ts

packages/perseus-core/src/parse-perseus-json/perseus-parsers/interactive-graph-widget.ts

jeremywiebe · 2025-04-11T14:43:57Z

packages/perseus-core/src/parse-perseus-json/perseus-parsers/measurer-widget.ts

@@ -32,7 +32,5 @@ export const parseMeasurerWidget: Parser<MeasurerWidget> = parseWidget(
        rulerPixels: number,
        rulerLength: number,
        box: pair(number, number),
-        // TODO(benchristel): static is not used. Remove it?
-        static: defaulted(boolean, () => false),


Nice! My guess is that this snuck into the object data at some point by accident.

jeremywiebe · 2025-04-11T14:44:41Z

packages/perseus/src/widgets/interactive-graph.tsx

@@ -163,7 +163,7 @@ type RenderProps = {
     * Shapes (points, chords, etc) displayed on the graph that cannot be moved
     * by the user.
     */
-    lockedFigures?: ReadonlyArray<LockedFigure>;
+    lockedFigures: ReadonlyArray<LockedFigure>;


Love that these parsers are giving us the ability to make the production code safer/cleaner!

handeyeco

Cool!

handeyeco · 2025-04-11T16:18:11Z

packages/perseus-core/src/data-schema.ts

@@ -1192,9 +1192,6 @@ export type PerseusMeasurerWidgetOptions = {
    rulerLength: number;
    // Containing area [width, height]
    box: [number, number];
-    // TODO(benchristel): static is not used. Remove it?
-    // Always false.  Not used for this widget
-    static: boolean;


Nice, static is so annoying

Myranae

Wanted to remind you to add back coords in interactive graph and to point out some checks need to be fixed. Also, since data-schema changed, we might need to inform CP, iirc? Just one NB question, but looks good otherwise :)

Myranae · 2025-04-14T14:32:02Z

packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx

-                        {props.lockedFigures && (
-                            <GraphLockedLabelsLayer
-                                lockedFigures={props.lockedFigures}
-                            />
-                        )}
+                        <GraphLockedLabelsLayer
+                            lockedFigures={props.lockedFigures}
+                        />


So before when lockedFigures was optional, if it was falsy in some way, then this GraphLockedLabelsLayer element wouldn't be included. Now it's always included. Could that cause any side effects with interaction or the UI?

I don't think this would render in the DOM if lockedFigures length is 0, because the outermost layer in packages/perseus/src/widgets/interactive-graphs/graph-locked-labels-layer.tsx is a React fragment. It only renders the actual labels themselves if there are any.

So I think this is fine.

nishasy

Nice! Looks good to me.

I added one question for my own understanding, but I don't anticipate it being a problem.

nishasy · 2025-04-15T19:49:19Z

packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx

-                        {props.lockedFigures && (
-                            <GraphLockedLabelsLayer
-                                lockedFigures={props.lockedFigures}
-                            />
-                        )}
+                        <GraphLockedLabelsLayer
+                            lockedFigures={props.lockedFigures}
+                        />


I don't think this would render in the DOM if lockedFigures length is 0, because the outermost layer in packages/perseus/src/widgets/interactive-graphs/graph-locked-labels-layer.tsx is a React fragment. It only renders the actual labels themselves if there are any.

So I think this is fine.

nishasy · 2025-04-15T19:50:59Z

packages/perseus-core/src/data-schema.ts

@@ -782,7 +782,7 @@ export type PerseusInteractiveGraphWidgetOptions = {
    correct: PerseusGraphType;
    // Shapes (points, chords, etc) displayed on the graph that cannot
    // be moved by the user.
-    lockedFigures?: ReadonlyArray<LockedFigure>;


Very exciting! The rest of the code for making lockedFigures required makes sense to me. Same for labels.

Only one question for my own understanding: Is there any possibility of this causing issues for old exercise items that saved this as null or undefined? Or would that be fixed by the parser?

That will be fixed up by the parser! It will default null and undefined to an empty array.

…s of PerseusInteractiveGraphWidgetOptions.lockedFigures

…e-test-tool

benchristel self-assigned this Apr 10, 2025

benchristel requested review from handeyeco, jeremywiebe, Myranae and nishasy April 10, 2025 23:14

benchristel added the olc-5.0.fb2b9 label Apr 10, 2025

jeremywiebe approved these changes Apr 11, 2025

View reviewed changes

handeyeco approved these changes Apr 11, 2025

View reviewed changes

Myranae approved these changes Apr 14, 2025

View reviewed changes

benchristel changed the base branch from benc/radio-parser-types to main April 14, 2025 22:16

nishasy approved these changes Apr 15, 2025

View reviewed changes

benchristel force-pushed the benc/parser-todos branch from 2577f55 to 09e5097 Compare April 17, 2025 22:25

benchristel changed the base branch from main to benc/idempotent-defaulted April 17, 2025 22:26

benchristel force-pushed the benc/idempotent-defaulted branch from bcb6197 to d36f830 Compare April 22, 2025 18:31

Base automatically changed from benc/idempotent-defaulted to main April 22, 2025 18:48

benchristel added 10 commits April 22, 2025 11:50

[benc/parser-todos] Resolve TODO comments

67766b1

[benc/parser-todos] Default labels and lockedFigures to empty

e38d4c5

[benc/parser-todos] Remove checks for undefined lockedFigures and labels

3997a89

[benc/parser-todos] Fix bug where was not parsed for point graphs

c8fe88c

[benc/parser-todos] Remove conditional checks for labels

3632183

[benc/parser-todos] Fix type error

e588e38

[benc/parser-todos] Fix lint

5cdb17d

[benc/parser-todos] Update tests

0d76a30

[benc/parser-todos] docs(changeset): Remove 'undefined' from the type…

e16c991

…s of PerseusInteractiveGraphWidgetOptions.lockedFigures

[benc/parser-todos] Hardcode encoding in parse-perseus-json/exhaustiv…

5482421

…e-test-tool

benchristel force-pushed the benc/parser-todos branch from 0ecb683 to 5482421 Compare April 22, 2025 18:51

benchristel merged commit 1f88cc1 into main Apr 22, 2025
8 checks passed

benchristel deleted the benc/parser-todos branch April 22, 2025 19:09

khan-actions-bot mentioned this pull request Apr 22, 2025

Version Packages #2412

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve some TODO comments in the Perseus JSON parsing code #2380

Resolve some TODO comments in the Perseus JSON parsing code #2380

benchristel commented Apr 10, 2025

github-actions bot commented Apr 10, 2025 •

edited

Loading

github-actions bot commented Apr 10, 2025 •

edited

Loading

jeremywiebe left a comment

jeremywiebe Apr 11, 2025

jeremywiebe Apr 11, 2025

handeyeco left a comment

handeyeco Apr 11, 2025

Myranae left a comment

Myranae Apr 14, 2025

nishasy Apr 15, 2025

nishasy left a comment

nishasy Apr 15, 2025

nishasy Apr 15, 2025

benchristel Apr 15, 2025

Resolve some TODO comments in the Perseus JSON parsing code #2380

Resolve some TODO comments in the Perseus JSON parsing code #2380

Conversation

benchristel commented Apr 10, 2025

Test plan:

github-actions bot commented Apr 10, 2025 • edited Loading

github-actions bot commented Apr 10, 2025 • edited Loading

npm Snapshot: Published

jeremywiebe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

handeyeco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myranae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishasy left a 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 Apr 10, 2025 •

edited

Loading

github-actions bot commented Apr 10, 2025 •

edited

Loading