Skip to content

Parse ELA radio widgets #2622

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.

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

Closed
wants to merge 1 commit into from
Closed

Parse ELA radio widgets #2622

wants to merge 1 commit into from

Conversation

benchristel
Copy link
Member

@benchristel benchristel commented Jun 18, 2025

In ELA exercises on Khan Academy, widgets are parsed by Go code and
re-serialized as JSON before being served to the frontend. The Go parser fills
in zero values for any missing fields, e.g. nil (which serializes to JSON
null). Previously our Radio widget parser didn't accept null values for
onePerLine and noneOfTheAbove; it wanted undefined. As a result, we saw
parse errors in ELA exercises when we tried to deploy the recent Radio v3
upgrade.

This PR fixes the parser so it can handle Radio widgets that have had null
filled in for onePerLine and noneOfTheAbove.

Issue: none

Test plan:

pnpm test

Copy link
Contributor

Size Change: +23 B (0%)

Total Size: 472 kB

Filename Size Change
packages/perseus-core/dist/es/index.js 20.5 kB +23 B (+0.11%)
ℹ️ 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-editor/dist/es/index.js 91.1 kB
packages/perseus-linter/dist/es/index.js 7.14 kB
packages/perseus-score/dist/es/index.js 9.23 kB
packages/perseus-utils/dist/es/index.js 403 B
packages/perseus/dist/es/index.js 200 kB
packages/perseus/dist/es/strings.js 7.56 kB
packages/pure-markdown/dist/es/index.js 1.22 kB
packages/simple-markdown/dist/es/index.js 6.71 kB

compressed-size-action

Copy link
Contributor

npm Snapshot: Published

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

Example:

pnpm add @khanacademy/perseus@PR2622

If you are working in Khan Academy's frontend, you can run the below command.

./tools/bump_perseus_version.js -t PR2622

If you are working in Khan Academy's webapp, you can run the below command.

./dev/tools/bump_perseus_version.js -s n -t PR2622

@@ -32,6 +32,12 @@ function getDefaultOptions() {
};
}

const parseOnePerLine = defaulted(optional(boolean), () => undefined);
const parseNoneOfTheAbove = defaulted(
optional(constant(false)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just make optional handle null too? My concern is that while this would solve the problem for Anakaren's work, the problem would resurface next time we make a similar schema change.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might cause bugs if we change the output type of the parsers though. Ideally, nothing should be treating null and undefined differently, but it's somewhat hard to confirm that.

Copy link
Member Author

@benchristel benchristel Jun 18, 2025

Choose a reason for hiding this comment

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

I created a separate ticket for treating undefined and null the same: https://khanacademy.atlassian.net/browse/LEMS-3232

That's going to be a bigger chunk of work.

@benchristel benchristel requested a review from handeyeco June 18, 2025 18:20
@benchristel
Copy link
Member Author

Closed in favor of #2640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants