Make values optional on PerseusCategorizerWidgetOptions type
#2123
Conversation
…optional on the PerseusCategorizerWidgetOptions type. This will allow us to remove `
| const values = categorizer.options?.values; | ||
| const categories = categorizer.options.categories; | ||
| const items = categorizer.options.items; | ||
| const values = categorizer.options.values; |
There was a problem hiding this comment.
These ?. nullish coalescing operators were not needed, since we check the nullishness of each of these fields above.
| @@ -28,13 +28,13 @@ import type {CategorizerPromptJSON} from "../../widget-ai-utils/categorizer/cate | |||
| import type {PerseusCategorizerWidgetOptions} from "@khanacademy/perseus-core"; | |||
|
|
|||
| type Props = WidgetProps<RenderProps, PerseusCategorizerScoringData> & { | |||
| values: ReadonlyArray<string>; | |||
| values: ReadonlyArray<number>; | |||
There was a problem hiding this comment.
This should have been ReadonlyArray<number> all along. Fixing this let me remove the @ts-expect-error below.
There was a problem hiding this comment.
I thought this might be the case, but I couldn't figure out why it was added at all. It's already included in renderProps, so I thought it might have been for another reason. Glad we can make it number! Do we even need that added on if it's already added via RenderProps?
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (e0885be) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2123If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2123 |
|
Size Change: -59 B (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
| @@ -423,7 +423,7 @@ export type PerseusCategorizerWidgetOptions = { | |||
| // Whether this widget is displayed with the results and immutable | |||
| static: boolean; | |||
| // The correct answers where index relates to the items and value relates to the category. e.g. [0, 1, 0, 1, 2] | |||
| values: ReadonlyArray<number>; | |||
| values?: ReadonlyArray<number>; | |||
There was a problem hiding this comment.
Potential concern here: with this change, the data-schema types no longer represent the JSON that actually gets persisted. The JSON should always have the values field, but the client won't always receive it.
If we wanted to fix this, we would have to decouple the widget prop types from these data-schema types. Though I think maybe they are already pretty much decoupled (in theory) by the transform and staticTransform functions. transform renders the widget with public-only data and staticTransform renders with public+private data.
There was a problem hiding this comment.
I think if the JSON should always have the values field, then it might be worth it to fix this. It seems like the data-schema and our contract with users is pretty important, and I'd rather it be as accurate as possible. Aren't we creating different types for the JSON versus what the client is getting though?
There was a problem hiding this comment.
Yeah, I agree it's important for the data-schema to accurately reflect the JSON. You are right, the JSON types and the prop types are going to be different in a SSS world. I'll post in Slack with more thoughts.
| values: Props["values"]; | ||
| values: ReadonlyArray<number>; |
There was a problem hiding this comment.
Is this necessary if you updated the type of values to be this anyway?
This is a proof of concept of how we might change the WidgetOptions types so we
can avoid sending "private data" like hints and answers to the client.
Issue: LEMS-2562
Test plan:
yarn test