Skip to content
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

Allow the 'key' field of Radio widgets to be null #2300

Merged
merged 3 commits into from
Mar 19, 2025

Conversation

benchristel
Copy link
Member

@benchristel benchristel commented Mar 13, 2025

This fixes a bug in the parser that caused it to incorrectly reject some production data.

Reported here: https://khanacademy.slack.com/archives/C01AZ9H8TTQ/p1741876983359149

Issue: none

Test plan:

yarn test

After deployment, the number of errors on Sentry mentioning At (root).question.widgets["radio 1"].key -- expected number, but got null
should decrease sharply.

@benchristel benchristel self-assigned this Mar 13, 2025
@benchristel benchristel requested a review from handeyeco March 13, 2025 20:12
Copy link
Contributor

github-actions bot commented Mar 13, 2025

Size Change: 0 B

Total Size: 876 kB

ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 39.7 kB
packages/keypad-context/dist/es/index.js 760 B
packages/kmath/dist/es/index.js 11.1 kB
packages/math-input/dist/es/index.js 77.5 kB
packages/math-input/dist/es/strings.js 1.79 kB
packages/perseus-core/dist/es/index.js 31.9 kB
packages/perseus-editor/dist/es/index.js 276 kB
packages/perseus-linter/dist/es/index.js 22.8 kB
packages/perseus-score/dist/es/index.js 20.6 kB
packages/perseus/dist/es/index.js 370 kB
packages/perseus/dist/es/strings.js 6.73 kB
packages/pure-markdown/dist/es/index.js 4.14 kB
packages/simple-markdown/dist/es/index.js 13.1 kB

compressed-size-action

Copy link
Contributor

github-actions bot commented Mar 13, 2025

npm Snapshot: Published

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

Example:

pnpm add @khanacademy/perseus@PR2300

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

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

@benchristel benchristel requested a review from Myranae March 13, 2025 22:00
@@ -45,7 +45,7 @@ export function parseWidgetWithVersion<
graded: optional(boolean),
alignment: optional(string),
options: parseOptions,
key: optional(number),
key: optional(nullable(number)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to your PR, but data-schema says key is:

Only used by interactive child widgets (line, point, etc) to identify the components

Any sense for why Radio has a value for this?

Copy link
Contributor

@Myranae Myranae left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! Glad we'll have fewer errors :)

@handeyeco
Copy link
Contributor

handeyeco commented Mar 17, 2025

I just realized I made a mistake skipping a test in the parser (I didn't mean to land this): https://github.com/Khan/perseus/pull/2233/files#diff-68d645e728532aa287208fa9587f48a0e0f8fcabd727a85d72f1d2e337b1024eR17

I think it's related to this work?

EDIT: looks like your changes only fix some of these tests. Ping me when you land this and I'll fix the others.

@benchristel benchristel merged commit fea65ea into main Mar 19, 2025
8 checks passed
@benchristel benchristel deleted the benc/allow-null-widget-keys branch March 19, 2025 15:28
SonicScrewdriver added a commit that referenced this pull request Mar 19, 2025
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @khanacademy/[email protected]

### Major Changes

- [#2303](#2303)
[`5e7a6084c`](5e7a608)
Thanks [@benchristel](https://github.com/benchristel)! - Drop support
for using KaTeX as a math renderer. You may encounter styling issues or
TeX syntax errors if you try to implement `PerseusDependencies.TeX`
using KaTeX.

### Minor Changes

- [#2284](#2284)
[`0d5ab0b2e`](0d5ab0b)
Thanks [@nishasy](https://github.com/nishasy)! - Add new labelLocation
value for Interactive Graphs


- [#875](#875)
[`9737eb497`](9737eb4)
Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Remove
deprecated khan-exercise.css


- [#2305](#2305)
[`af6d89007`](af6d890)
Thanks [@Myranae](https://github.com/Myranae)! - Add a story for
Dropdown that uses answerless data

### Patch Changes

- [#2307](#2307)
[`dd7b13a78`](dd7b13a)
Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! -
Revert SSS temporarily for Radio to ensure Multiselect + Random answer
correctness is accurate

- Updated dependencies
\[[`0d5ab0b2e`](0d5ab0b),
[`fea65eaf1`](fea65ea)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Major Changes

- [#2303](#2303)
[`5e7a6084c`](5e7a608)
Thanks [@benchristel](https://github.com/benchristel)! - Drop support
for using KaTeX as a math renderer. You may encounter styling issues or
TeX syntax errors if you try to implement `PerseusDependencies.TeX`
using KaTeX.

### Minor Changes

- [#2284](#2284)
[`0d5ab0b2e`](0d5ab0b)
Thanks [@nishasy](https://github.com/nishasy)! - Add new labelLocation
value for Interactive Graphs

### Patch Changes

- Updated dependencies
\[[`0d5ab0b2e`](0d5ab0b),
[`fea65eaf1`](fea65ea),
[`9737eb497`](9737eb4),
[`dd7b13a78`](dd7b13a),
[`af6d89007`](af6d890),
[`5e7a6084c`](5e7a608)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Minor Changes

- [#2284](#2284)
[`0d5ab0b2e`](0d5ab0b)
Thanks [@nishasy](https://github.com/nishasy)! - Add new labelLocation
value for Interactive Graphs

### Patch Changes

- [#2300](#2300)
[`fea65eaf1`](fea65ea)
Thanks [@benchristel](https://github.com/benchristel)! - Bugfix: allow
the 'key' field of Radio widgets to be null when parsing Perseus JSON

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`0d5ab0b2e`](0d5ab0b),
[`fea65eaf1`](fea65ea)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`0d5ab0b2e`](0d5ab0b),
[`fea65eaf1`](fea65ea)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`0d5ab0b2e`](0d5ab0b),
[`fea65eaf1`](fea65ea)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`0d5ab0b2e`](0d5ab0b),
[`fea65eaf1`](fea65ea)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`0d5ab0b2e`](0d5ab0b),
[`fea65eaf1`](fea65ea)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

- Updated dependencies
\[[`0d5ab0b2e`](0d5ab0b),
[`fea65eaf1`](fea65ea)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants