SSS: Improve types for validation #2002

jeremywiebe · 2024-12-13T02:07:47Z

Summary:

This PR begins working out the types for Scoring, Validation, and how they relate to our full widget options types. There are currently three "trees" of types that are shaped as a map of widgetId to something. They are:

Full widget options (starting from PerseusRenderer)
Scoring data - used to score the learner's guess (user input)
Validation data - a shared subset (of Render and Scoring data) used to do empty widget checking (aka validation). This helps the frontend to know if the question is scorable yet.

Finally, there is also a widget map known as User Input. This map is a map of widget ids from the item to the user input the learner has entered so far.

Issue: LEMS-2561

Test plan:

yarn typecheck (especially, the new validation.typetest.ts file!)
yarn test (just to be sure)

github-actions · 2024-12-13T02:10:36Z

Size Change: +115 B (+0.01%)

Total Size: 1.27 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	416 kB	+115 B (+0.03%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	77.9 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	688 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	4.12 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

jeremywiebe · 2024-12-16T19:27:30Z

packages/perseus/src/validation.types.ts

-    | PerseusSorterRubric
-    | PerseusTableRubric;
-
-export type UserInput =


This is moved down below all of the Rubric/Scoring Data types together with the various widget UserInput types. #organizing

jeremywiebe · 2024-12-16T19:47:12Z

packages/perseus/src/validation.types.ts

+export interface RubricRegistry {
+    categorizer: PerseusCategorizerScoringData;
+    "cs-program": PerseusCSProgramRubric;
+    // definition: PerseusDefinitionScoringData;


I really am not a fan of how this works right now. The fact that this is not a full listing of all widgets because we don't score all widgets worries me, but I don't think it makes sense to add a type for all widgets just to make this "whole".

I'm tempted to leave it like this for now and as we work further hopefully something resolves to a better solution.

Also open to ideas.

If not all widgets have ScoringData, why don't we just remove the inner comments and add a comment to the whole interface (just above line 241) saying that? We could keep a list of which widgets currently don't have scoring data in that comment so it doesn't muddy the interface so to speak. Or is there a reason why we should have an entry for every widget in this interface?

My vote is remove entries for widgets we don't score.

github-actions · 2024-12-16T19:58:09Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR2002

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

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

Myranae · 2024-12-17T15:52:36Z

packages/perseus/src/__tests__/validation.typetest.ts

+// We can use a RubricMap as a ValidationDataMap
+0 as any as RubricMap satisfies ValidationDataMap;


May want to update these to ScoringMap since we're removing the rubric terminology.

Myranae · 2024-12-17T15:55:10Z

packages/perseus/src/perseus-types.ts

+// A utility type that constructs a widget map from a "registry interface".
+// The keys of the registry should be the widget type (aka, "categorizer" or
+// "radio", etc) and the value should be the option type stored in the value
+// of the map.


Could you provide an example of the output from this function? Or an example usage?

Yes. So I've started developing the idea of a type registry (because we have full widget options types, scoring data types, frontend rendering types). Each of these is map of widget type to the option type for that widget.

If you look at the usage in this PR, we use the PerseusWidgetTypes interface to create the PerseusWidgetsMap type.

PerseusWidgetTypes looks like this (shortened a bit here):

export interface PerseusWidgetTypes { categorizer: CategorizerWidget; "cs-program": CSProgramWidget; definition: DefinitionWidget; ... }

We use the helper:

export type PerseusWidgetsMap = MakeWidgetMap<PerseusWidgetTypes>;

And out comes the following type:

type PerseusWidgetsMap = { [x: `categorizer ${number}`]: CategorizerWidget; [x: `cs-program ${number}`]: CSProgramWidget; [x: `definition ${number}`]: DefinitionWidget; ... }

So you can think of this helper as a sort of type "generator". It takes a registry of widget types and generates a map type where the ID of the map is a widget id and the value is the correct option type.

Myranae · 2024-12-17T16:06:05Z

packages/perseus/src/validation.types.ts

-    | PerseusTableUserInput;
-
-export type UserInputMap = {[widgetId: string]: UserInput};
+export interface RubricRegistry {


Should this be ScoringDataRegistry? Doesn't have the same ring, but we're trying to move away from the rubric vocab, right? Also, should the Rubric types below be changed to ScoringData types or will we just handle that after landing my renaming PR when we get back? Might make sense to plan the order in which we land PRs, though it might make sense just to end the week with a fully merged feature branch with all our changes combined.

I plan to land this PR after your scoring name changes... so I'll be sure to update the instances of Rubric in this PR before landing.

handeyeco · 2024-12-17T16:44:37Z

packages/perseus/src/__tests__/validation.typetest.ts

+import type {RubricMap, ValidationDataMap} from "../validation.types";
+
+// We can use a 'widgets' map from a PerseusRenderer as a ValidationDataMap
+0 as any as PerseusRenderer["widgets"] satisfies ValidationDataMap;


This 0 as any as business really confused me when I first saw it; even now that I now what it's for, I still don't understand what it actually works. I honestly don't feel super comfortable having it in the code because it's both unclear and feels hacky. That being said, I'm fine if we keep it but I'd like it if we could link to some documentation on this pattern (or if it doesn't exist we can write our own).

This is a pattern I saw in Ben's parsing code changes.

I've tried a different approach that will hopefully be clearer. My concern is that the two types being tested here (PerseusRenderer["widgets"] and ScoringDataMap) must remain compatible with ValidatioDataMap.

I don't another way to do that.

handeyeco · 2024-12-17T16:49:07Z

packages/perseus/src/validation.types.ts

+export interface RubricRegistry {
+    categorizer: PerseusCategorizerScoringData;
+    "cs-program": PerseusCSProgramRubric;
+    // definition: PerseusDefinitionScoringData;


My vote is remove entries for widgets we don't score.

handeyeco · 2024-12-17T17:06:11Z

packages/perseus/src/validation.types.ts

+ * share functionality that understands how to traverse maps of `widget id` to
+ * `options`.
+ */
+export type RubricMap = {


So overall the point of this PR seems to be aimed at more closely matching widget-specific data to the widget ID?

Doesn't this reinforce the thing you and Nicole were against - tightly binding the widget type and the widget id? Like if we were to decide tomorrow that we were going to give all widgets UUIDs (which we should as we've seen in the GradedGroup issue), wouldn't that break this code?

I'm having trouble aligning the description of the PR and the ticket with the actual changes. This is like pre/post work for LEMS-2561?

Sorry, this PR drifted from what I initially typed out as the PR description. I'll update the description. It's basically type work to make the types we already use (full widget options, Scoring Data, Validation Data) a bit more clear and well-typed.

I agree that I don't like us buying further into the mapping of widget ID to data in our various maps. Maybe we can chat about that in a future PR? I'm also happy to have pushback and do more work in this PR to isolate the Renderer and outer code knowing so much about widget option details.

Myranae · 2024-12-17T20:26:27Z

packages/perseus/src/validation.types.ts

+export type RubricMap = {
+    [Property in keyof RubricRegistry as `${Property} ${number}`]: {
+        type: Property;
+        static?: boolean;
+        options: RubricRegistry[Property];
+    };
+};
+
+export type Rubric = RubricRegistry[keyof RubricRegistry];


Also pointing out the use of Rubric instead of ScoringData here as well.

Myranae · 2024-12-17T20:27:40Z

packages/perseus/src/validation.types.ts

+// A union type of all the widget user input types
+export type UserInput = UserInputRegisry[keyof UserInputRegisry];
+
+/**
+ * A map of widget IDs to user input types (strongly typed based on the format
+ * of the widget ID).
+ */
+export type UserInputMap = MakeWidgetMap<UserInputRegisry>;


Do you only use the asterisk comments in certain situations? Just wondering when it should be the // version or the /** version.

Oops, I should be consistent. I use /** */ for comments that would be useful to see if I used the type somewhere else.

Eg.

packages/perseus/src/validation.types.ts

Co-authored-by: Matthew <[email protected]>

jeremywiebe self-assigned this Dec 13, 2024

jeremywiebe force-pushed the jer/client-validation-3 branch from 91dd867 to c619dd8 Compare December 13, 2024 23:24

jeremywiebe changed the base branch from jer/client-validation-3 to feature/client-validation December 14, 2024 01:40

jeremywiebe force-pushed the jer/client-validation-4 branch from d84a790 to 8629c57 Compare December 16, 2024 19:17

jeremywiebe commented Dec 16, 2024

View reviewed changes

jeremywiebe marked this pull request as ready for review December 16, 2024 20:03

jeremywiebe requested review from handeyeco, Myranae and a team December 16, 2024 20:04

Myranae reviewed Dec 17, 2024

View reviewed changes

handeyeco reviewed Dec 17, 2024

View reviewed changes

Myranae reviewed Dec 17, 2024

View reviewed changes

handeyeco reviewed Dec 17, 2024

View reviewed changes

packages/perseus/src/validation.types.ts Outdated Show resolved Hide resolved

jeremywiebe added 7 commits December 18, 2024 16:56

Improve types for validation

8e5e778

Changeset

5bc363b

Revert @deprecated change to just deprecated

c039d47

REmove unused eslint suppression

606a9b7

Fix comment

e5c8bc6

Specify return type - for some reason TypeScript needs this

da93805

Fixing more names/usages of Rubric to be ScoringData

7ce25ce

jeremywiebe force-pushed the jer/client-validation-4 branch from 930bb96 to 7ce25ce Compare December 19, 2024 01:00

jeremywiebe and others added 3 commits December 18, 2024 17:05

Update packages/perseus/src/validation.types.ts

2f1fb62

Co-authored-by: Matthew <[email protected]>

Migrate comments to /** */ type JSDoc comments

a3eed89

Rework type tests - clearer?

cf7d5c1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSS: Improve types for validation #2002

SSS: Improve types for validation #2002

jeremywiebe commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

jeremywiebe Dec 16, 2024

jeremywiebe Dec 16, 2024

Myranae Dec 17, 2024

handeyeco Dec 17, 2024

github-actions bot commented Dec 16, 2024 •

edited

Loading

Myranae Dec 17, 2024

Myranae Dec 17, 2024

jeremywiebe Dec 19, 2024

Myranae Dec 17, 2024

jeremywiebe Dec 19, 2024

handeyeco Dec 17, 2024

Myranae Dec 18, 2024

jeremywiebe Dec 19, 2024

handeyeco Dec 17, 2024

handeyeco Dec 17, 2024

jeremywiebe Dec 19, 2024

Myranae Dec 17, 2024

Myranae Dec 17, 2024

jeremywiebe Dec 19, 2024

		// We can use a RubricMap as a ValidationDataMap
		0 as any as RubricMap satisfies ValidationDataMap;

SSS: Improve types for validation #2002

Are you sure you want to change the base?

SSS: Improve types for validation #2002

Conversation

jeremywiebe commented Dec 13, 2024 • edited Loading

Summary:

Test plan:

github-actions bot commented Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 16, 2024 • edited Loading

npm Snapshot: Published

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremywiebe commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024 •

edited

Loading