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.

Sign up for GitHub

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

Make values optional on PerseusCategorizerWidgetOptions type #2123

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Make values optional on PerseusCategorizerWidgetOptions type #2123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a13e72f
[benc/categorizer-options-type] Make 'values' optional on categorizer…
benchristel Jan 16, 2025
780a8b8
[benc/categorizer-options-type] docs(changeset): Make 'values' field …
benchristel Jan 16, 2025
e0885be
[benc/categorizer-options-type] Fix lint
benchristel Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/witty-suns-mate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/perseus": minor
"@khanacademy/perseus-core": minor
---

Make `values` field optional on the `PerseusCategorizerWidgetOptions` type. This will allow us to remove `values` (which represents the correct answer) from the data sent to the client.
2 changes: 1 addition & 1 deletion packages/perseus-core/src/data-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

// Whether we should highlight i18n linter errors found on this widget
highlightLint?: boolean;
// Internal editor configuration. Can be ignored by consumers.
Expand Down
6 changes: 3 additions & 3 deletions packages/perseus/src/util/extract-perseus-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ function getAnswersFromWidgets(
categorizer.options?.items &&
categorizer.options?.values
) {
const categories = categorizer.options?.categories;
const items = categorizer.options?.items;
const values = categorizer.options?.values;
const categories = categorizer.options.categories;
const items = categorizer.options.items;
const values = categorizer.options.values;
Copy link
Member Author

Choose a reason for hiding this comment

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

These ?. nullish coalescing operators were not needed, since we check the nullishness of each of these fields above.

answers.push(
...values.map(
(value, index) =>
Expand Down
32 changes: 32 additions & 0 deletions packages/perseus/src/util/parse-perseus-json/perseus-parsers/categorizer-widget.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import {parse} from "../parse";
import {success} from "../result";

import {parseCategorizerWidget} from "./categorizer-widget";

import type {CategorizerWidget} from "@khanacademy/perseus-core";

describe("parseCategorizerWidget", () => {
const baseWidget: CategorizerWidget = {
type: "categorizer",
version: {major: 0, minor: 0},
graded: true,
options: {
items: [],
categories: [],
randomizeItems: false,
static: false,
},
};

it("allows `values` to be undefined", () => {
const widget = {
...baseWidget,
options: {
...baseWidget.options,
values: undefined,
},
};
const result = parse(widget, parseCategorizerWidget);
expect(result).toEqual(success(widget));
});
});
2 changes: 1 addition & 1 deletion packages/perseus/src/util/parse-perseus-json/perseus-parsers/categorizer-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export const parseCategorizerWidget: Parser<CategorizerWidget> = parseWidget(
categories: array(string),
randomizeItems: defaulted(boolean, () => false),
static: defaulted(boolean, () => false),
values: array(defaulted(number, () => 0)),
values: optional(array(defaulted(number, () => 0))),
highlightLint: optional(boolean),
linterContext: optional(
object({
Expand Down
5 changes: 2 additions & 3 deletions packages/perseus/src/widgets/categorizer/categorizer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Copy link
Member Author

Choose a reason for hiding this comment

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

This should have been ReadonlyArray<number> all along. Fixing this let me remove the @ts-expect-error below.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

};

type DefaultProps = {
items: Props["items"];
categories: Props["categories"];
values: Props["values"];
values: ReadonlyArray<number>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary if you updated the type of values to be this anyway?

linterContext: Props["linterContext"];
};

Expand Down Expand Up @@ -79,7 +79,6 @@ export class Categorizer

onChange(itemNum, catNum) {
const values = [...this.props.values];
// @ts-expect-error - TS2322 - Type 'number' is not assignable to type 'never'.
values[itemNum] = catNum;
this.change("values", values);
this.props.trackInteraction();
Expand Down
Loading