-
Notifications
You must be signed in to change notification settings - Fork 357
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
Parse ELA radio widgets #2622
Conversation
Size Change: +23 B (0%) Total Size: 472 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (e30e72d) and published it to npm. You 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)), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Closed in favor of #2640 |
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 JSONnull
). Previously our Radio widget parser didn't acceptnull
values foronePerLine
andnoneOfTheAbove
; it wantedundefined
. As a result, we sawparse 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
andnoneOfTheAbove
.Issue: none
Test plan:
pnpm test