[Client Validation] Fixes after merging main #2122

jeremywiebe · 2025-01-16T22:17:51Z

Summary:

I recently merged main and the introduction of MockWidget caused some tests to break. I've fixed them in this PR and adjusted the MockWidget to be simpler (ie. not use KhanAnswerTypes) as I don't think that's needed.

I also fixed up some related types so that the MockWidget does not exist in our production Widget types, only in tests. The new MockWidget illustrates this "test-only" expansion of the widget types.

Issue: LEMS-2561

Test plan:

yarn test
yarn typecheck

…e unions and maps

github-actions · 2025-01-16T22:20:35Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR2122

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

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

github-actions · 2025-01-16T22:20:43Z

Size Change: +120 B (+0.01%)

Total Size: 1.47 MB

Filename	Size	Change
`packages/perseus-core/dist/es/index.js`	23.6 kB	+120 B (+0.51%)

ℹ️ 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`	83.1 kB
`packages/math-input/dist/es/index.js`	78 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-editor/dist/es/index.js`	689 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus-score/dist/es/index.js`	103 kB
`packages/perseus/dist/es/index.js`	412 kB
`packages/perseus/dist/es/strings.js`	4.82 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 · 2025-01-16T22:29:57Z

packages/perseus-core/src/data-schema.ts

@@ -144,7 +144,6 @@ export interface PerseusWidgetTypes {
    matcher: MatcherWidget;
    matrix: MatrixWidget;
    measurer: MeasurerWidget;
-    "mock-widget": MockWidget;


Removing here as we don't want the MockWidget to be part of the public set of widget types. It is an internal, test-only widget.

jeremywiebe · 2025-01-16T22:30:28Z

packages/perseus-core/src/data-schema.ts

@@ -346,8 +345,6 @@ export type MatrixWidget = WidgetOptions<'matrix', PerseusMatrixWidgetOptions>;
 // prettier-ignore
 export type MeasurerWidget = WidgetOptions<'measurer', PerseusMeasurerWidgetOptions>;
 // prettier-ignore
-export type MockWidget = WidgetOptions<'mock-widget', MockWidgetOptions>;


I moved all types related to the MockWidget into its directory so that we don't have test pieces intermingled/shipped with our production code.

jeremywiebe · 2025-01-16T22:31:24Z

packages/perseus-core/src/data-schema.ts

-    | TableWidget
-    | VideoWidget
-    | DeprecatedStandinWidget;
+export type PerseusWidget = PerseusWidgetTypes[keyof PerseusWidgetTypes];


Now that we have the PerseusWidgetTypes interface (which can be properly extended in test code and/or externally), we can derive this union from that (meaning we don't have to maintain two official set of widget types.

jeremywiebe · 2025-01-16T22:39:39Z

packages/perseus/src/__testdata__/server-item-renderer.testdata.ts

@@ -40,7 +41,7 @@ export const itemWithNumericInput: PerseusItem = {
                    labelText: "What's the answer?",
                    size: "normal",
                },
-            } as NumericInputWidget,
+            } satisfies NumericInputWidget,


satisfies

The new satisfies operator lets us validate that the type of an expression matches some type, without changing the resulting type of that expression.

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-9.html#the-satisfies-operator

Compare to as which does some type checking but is not as robust and can end up hiding test data that doesn't actually fully match the as type.

jeremywiebe · 2025-01-16T22:40:31Z

packages/perseus/src/widgets/mock-widgets/mock-widget-types.ts

+declare module "@khanacademy/perseus-core" {
+    export interface PerseusWidgetTypes {
+        "mock-widget": MockWidget;
+    }
+}


This is the magic sauce that extends the set of PerseusWidgetTypes to include the mock widget. Anything that ends up importing this file will "see" the mock-widget... meaning tests, but not production code!

I don't really understand the code and after looking it up I still don't understand the code. I wonder if would be worth adding a link to something that explains the goal here.

I'll expand the comment. Honestly, I wasn't able to find a really clear description for the case we're using this for. The bottom line is that when this file is imported (should be from tests only), the PerseusWidgetTypes type from @khanacademy/perseus-core is extended to include the MockWidget. Our other types that define the unions of widget options types, etc. are all based on PerseusWidgetTypes. So once we extend PerseusWidgetTypes with this mock widget type, all the other widget option unions know about the mock widget also. This means that our tests and test data understand that mock-widget 1 refers to a mock widget, but if you were to try it in webapp, it wouldn't know about mock widgets (rightfully).

… be seen only in tests)

jeremywiebe · 2025-01-16T22:42:48Z

packages/perseus-core/src/data-schema.ts

@@ -380,43 +389,6 @@ export type VideoWidget = WidgetOptions<'video', PerseusVideoWidgetOptions>;
 //prettier-ignore
 export type DeprecatedStandinWidget = WidgetOptions<'deprecated-standin', object>;

-export type PerseusWidget =


Moved up to Line 183.

handeyeco · 2025-01-21T16:44:03Z

packages/perseus/src/widgets/mock-widgets/mock-widget.tsx

@@ -6,18 +6,15 @@ import * as React from "react";
 import {getPromptJSON as _getPromptJSON} from "../../widget-ai-utils/mock-widget/prompt-utils";

 import scoreMockWidget from "./score-mock-widget";
+import validateMockWidget from "./validate-mock-widget";


Not related to your PR, but I wonder if all this mock widget stuff should even be in /packages when it could possibly live in /testing.

handeyeco · 2025-01-21T16:50:12Z

packages/perseus/src/widgets/mock-widgets/mock-widget-types.ts

+declare module "@khanacademy/perseus-core" {
+    export interface PerseusWidgetTypes {
+        "mock-widget": MockWidget;
+    }
+}


I don't really understand the code and after looking it up I still don't understand the code. I wonder if would be worth adding a link to something that explains the goal here.

handeyeco · 2025-01-21T16:51:30Z

packages/perseus/src/widget-ai-utils/mock-widget/prompt-utils.ts

 import type mockWidget from "../../widgets/mock-widgets/mock-widget";
+import type {PerseusMockWidgetUserInput} from "../../widgets/mock-widgets/mock-widget-types";


Why do we need AI prompt utils for a mock widget?

I think just to "round out" the implementation of the mock widget. I don't think we actually use it. Perhaps it'd be better if we deleted this to avoid confusion and have folks think it serves some purpose.

Changed my mind. There's tests that exercise the Renderer calling the widget's getPromptJSON() using the mock widget. I'll leave it in place as that feels like a valid use of the mock widget. I'd also like to avoid doing this sort of refactoring in this PR as I'm trying to stick to just dealing with after effects from merging main. :)

handeyeco · 2025-01-21T16:56:20Z

packages/perseus-core/src/data-schema.ts

+ *
+ * @see MockWidget for an example
+ */
+export type PerseusWidget = PerseusWidgetTypes[keyof PerseusWidgetTypes];


jeremywiebe added 2 commits January 15, 2025 11:24

Simplify MockWidget and move its types to be private to tests-only

0f7d4e6

Type fixes so the mock-widget isn't part of our production widget typ…

c099e9e

…e unions and maps

jeremywiebe self-assigned this Jan 16, 2025

jeremywiebe commented Jan 16, 2025

View reviewed changes

jeremywiebe added 2 commits January 16, 2025 14:41

docs(changeset): Type and test fixes for new MockWidget (isolating to…

9f40a53

… be seen only in tests)

Changeset

29fba55

jeremywiebe commented Jan 16, 2025

View reviewed changes

jeremywiebe marked this pull request as ready for review January 16, 2025 22:43

jeremywiebe requested review from benchristel, Myranae and handeyeco January 17, 2025 20:57

handeyeco approved these changes Jan 21, 2025

View reviewed changes

Hopefully clarify extending the widget types interface

75bab8b

jeremywiebe merged commit 1a75ca6 into feature/client-validation Jan 21, 2025
8 checks passed

jeremywiebe deleted the jer/client-validation-merge-fixes branch January 21, 2025 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Client Validation] Fixes after merging main #2122

[Client Validation] Fixes after merging main #2122

jeremywiebe commented Jan 16, 2025

github-actions bot commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

jeremywiebe Jan 16, 2025

jeremywiebe Jan 16, 2025

jeremywiebe Jan 16, 2025

jeremywiebe Jan 16, 2025

jeremywiebe Jan 16, 2025

handeyeco Jan 21, 2025

jeremywiebe Jan 21, 2025

jeremywiebe Jan 16, 2025

handeyeco Jan 21, 2025

handeyeco Jan 21, 2025

handeyeco Jan 21, 2025

jeremywiebe Jan 21, 2025

jeremywiebe Jan 21, 2025

handeyeco Jan 21, 2025

		import type mockWidget from "../../widgets/mock-widgets/mock-widget";
		import type {PerseusMockWidgetUserInput} from "../../widgets/mock-widgets/mock-widget-types";

[Client Validation] Fixes after merging main #2122

[Client Validation] Fixes after merging main #2122

Conversation

jeremywiebe commented Jan 16, 2025

Summary:

Test plan:

github-actions bot commented Jan 16, 2025 • edited Loading

npm Snapshot: Published

github-actions bot commented Jan 16, 2025 • 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

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

github-actions bot commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading