-
Notifications
You must be signed in to change notification settings - Fork 350
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
[Client Validation] Fixes after merging main #2122
Changes from all commits
0f7d4e6
c099e9e
9f40a53
29fba55
75bab8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
"@khanacademy/perseus": patch | ||
"@khanacademy/perseus-core": patch | ||
"@khanacademy/perseus-editor": patch | ||
--- | ||
|
||
Type and test fixes for new MockWidget (isolating to be seen only in tests) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,7 +102,7 @@ export type MakeWidgetMap<TRegistry> = { | |
* `PerseusWidgets` with the one defined below. | ||
* | ||
* ```typescript | ||
* declare module "@khanacademy/perseus" { | ||
* declare module "@khanacademy/perseus-core" { | ||
* interface PerseusWidgetTypes { | ||
* // A new widget | ||
* "new-awesomeness": MyAwesomeNewWidget; | ||
|
@@ -144,7 +144,6 @@ export interface PerseusWidgetTypes { | |
matcher: MatcherWidget; | ||
matrix: MatrixWidget; | ||
measurer: MeasurerWidget; | ||
"mock-widget": MockWidget; | ||
"molecule-renderer": MoleculeRendererWidget; | ||
"number-line": NumberLineWidget; | ||
"numeric-input": NumericInputWidget; | ||
|
@@ -181,6 +180,18 @@ export interface PerseusWidgetTypes { | |
*/ | ||
export type PerseusWidgetsMap = MakeWidgetMap<PerseusWidgetTypes>; | ||
|
||
/** | ||
* PerseusWidget is a union of all the different types of widget options that | ||
* Perseus knows about. | ||
* | ||
* Thanks to it being based on PerseusWidgetTypes interface, this union is | ||
* automatically extended to include widgets used in tests without those widget | ||
* option types seeping into our production types. | ||
* | ||
* @see MockWidget for an example | ||
*/ | ||
export type PerseusWidget = PerseusWidgetTypes[keyof PerseusWidgetTypes]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice |
||
|
||
/** | ||
* A "PerseusItem" is a classic Perseus item. It is rendered by the | ||
* `ServerItemRenderer` and the layout is pre-set. | ||
|
@@ -346,8 +357,6 @@ export type MatrixWidget = WidgetOptions<'matrix', PerseusMatrixWidgetOptions>; | |
// prettier-ignore | ||
export type MeasurerWidget = WidgetOptions<'measurer', PerseusMeasurerWidgetOptions>; | ||
// prettier-ignore | ||
export type MockWidget = WidgetOptions<'mock-widget', MockWidgetOptions>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// prettier-ignore | ||
export type NumberLineWidget = WidgetOptions<'number-line', PerseusNumberLineWidgetOptions>; | ||
// prettier-ignore | ||
export type NumericInputWidget = WidgetOptions<'numeric-input', PerseusNumericInputWidgetOptions>; | ||
|
@@ -380,43 +389,6 @@ export type VideoWidget = WidgetOptions<'video', PerseusVideoWidgetOptions>; | |
//prettier-ignore | ||
export type DeprecatedStandinWidget = WidgetOptions<'deprecated-standin', object>; | ||
|
||
export type PerseusWidget = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved up to Line 183. |
||
| CategorizerWidget | ||
| CSProgramWidget | ||
| DefinitionWidget | ||
| DropdownWidget | ||
| ExplanationWidget | ||
| ExpressionWidget | ||
| GradedGroupSetWidget | ||
| GradedGroupWidget | ||
| GrapherWidget | ||
| GroupWidget | ||
| IFrameWidget | ||
| ImageWidget | ||
| InputNumberWidget | ||
| InteractionWidget | ||
| InteractiveGraphWidget | ||
| LabelImageWidget | ||
| MatcherWidget | ||
| MatrixWidget | ||
| MeasurerWidget | ||
| MockWidget | ||
| MoleculeRendererWidget | ||
| NumberLineWidget | ||
| NumericInputWidget | ||
| OrdererWidget | ||
| PassageRefWidget | ||
| PassageWidget | ||
| PhetSimulationWidget | ||
| PlotterWidget | ||
| PythonProgramWidget | ||
| RadioWidget | ||
| RefTargetWidget | ||
| SorterWidget | ||
| TableWidget | ||
| VideoWidget | ||
| DeprecatedStandinWidget; | ||
|
||
/** | ||
* A background image applied to various widgets. | ||
*/ | ||
|
@@ -1720,11 +1692,6 @@ export type PerseusVideoWidgetOptions = { | |
static?: boolean; | ||
}; | ||
|
||
export type MockWidgetOptions = { | ||
static?: boolean; | ||
value: string; | ||
}; | ||
|
||
export type PerseusInputNumberWidgetOptions = { | ||
answerType?: | ||
| "number" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,10 @@ import { | |
type ExpressionWidget, | ||
type RadioWidget, | ||
type NumericInputWidget, | ||
type MockWidget, | ||
} from "@khanacademy/perseus-core"; | ||
|
||
import type {MockWidget} from "../widgets/mock-widgets/mock-widget-types"; | ||
|
||
export const itemWithNumericInput: PerseusItem = { | ||
question: { | ||
content: | ||
|
@@ -40,7 +41,7 @@ export const itemWithNumericInput: PerseusItem = { | |
labelText: "What's the answer?", | ||
size: "normal", | ||
}, | ||
} as NumericInputWidget, | ||
} satisfies NumericInputWidget, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Compare to |
||
}, | ||
}, | ||
hints: [ | ||
|
@@ -64,7 +65,7 @@ export const itemWithMockWidget: PerseusItem = { | |
options: { | ||
value: "3", | ||
}, | ||
} as MockWidget, | ||
} satisfies MockWidget, | ||
}, | ||
}, | ||
hints: [ | ||
|
@@ -158,14 +159,14 @@ export const itemWithTwoMockWidgets: PerseusItem = { | |
options: { | ||
value: "3", | ||
}, | ||
} as MockWidget, | ||
} satisfies MockWidget, | ||
"mock-widget 2": { | ||
type: "mock-widget", | ||
graded: true, | ||
options: { | ||
value: "3", | ||
}, | ||
} as MockWidget, | ||
} satisfies MockWidget, | ||
}, | ||
}, | ||
hints: [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import type {PerseusMockWidgetUserInput} from "../../widgets/mock-widgets/mock-widget"; | ||
import type mockWidget from "../../widgets/mock-widgets/mock-widget"; | ||
import type {PerseusMockWidgetUserInput} from "../../widgets/mock-widgets/mock-widget-types"; | ||
import type React from "react"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need AI prompt utils for a mock widget? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed my mind. There's tests that exercise the |
||
|
||
export type MockWidgetPromptJSON = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import type {WidgetOptions} from "@khanacademy/perseus-core"; | ||
|
||
export type MockWidget = WidgetOptions<"mock-widget", MockWidgetOptions>; | ||
|
||
export type MockWidgetOptions = { | ||
static?: boolean; | ||
value: string; | ||
}; | ||
|
||
export type PerseusMockWidgetRubric = { | ||
value: string; | ||
}; | ||
|
||
export type PerseusMockWidgetUserInput = { | ||
currentValue: string; | ||
}; | ||
|
||
// Extend the widget registries for testing | ||
// See @khanacademy/perseus-core's PerseusWidgetTypes for a full explanation. | ||
// Basically, we're extending the interface from that package so that our | ||
// testing code knows of the MockWidget. In production code, there's no | ||
// knowledge of the mock widget. | ||
declare module "@khanacademy/perseus-core" { | ||
export interface PerseusWidgetTypes { | ||
"mock-widget": MockWidget; | ||
} | ||
} | ||
Comment on lines
+23
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the magic sauce that extends the set of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not related to your PR, but I wonder if all this mock widget stuff should even be in |
||
|
||
import type { | ||
MockWidgetOptions, | ||
PerseusMockWidgetRubric, | ||
PerseusMockWidgetUserInput, | ||
} from "./mock-widget-types"; | ||
import type {WidgetExports, WidgetProps, Widget, FocusPath} from "../../types"; | ||
import type {MockWidgetPromptJSON} from "../../widget-ai-utils/mock-widget/prompt-utils"; | ||
import type {MockWidgetOptions} from "@khanacademy/perseus-core"; | ||
|
||
export type PerseusMockWidgetRubric = { | ||
value: string; | ||
}; | ||
|
||
export type PerseusMockWidgetUserInput = { | ||
currentValue: string; | ||
}; | ||
|
||
type ExternalProps = WidgetProps<MockWidgetOptions, PerseusMockWidgetRubric>; | ||
|
||
|
@@ -41,7 +38,7 @@ type Props = ExternalProps & { | |
* | ||
* You can register this widget for your tests by calling `registerWidget("mock-widget", MockWidget);` | ||
*/ | ||
export class MockWidget extends React.Component<Props> implements Widget { | ||
class MockWidgetComponent extends React.Component<Props> implements Widget { | ||
static defaultProps: DefaultProps = { | ||
currentValue: "", | ||
}; | ||
|
@@ -93,7 +90,7 @@ export class MockWidget extends React.Component<Props> implements Widget { | |
}; | ||
|
||
getUserInput(): PerseusMockWidgetUserInput { | ||
return MockWidget.getUserInputFromProps(this.props); | ||
return MockWidgetComponent.getUserInputFromProps(this.props); | ||
} | ||
|
||
handleChange: ( | ||
|
@@ -131,9 +128,12 @@ const styles = StyleSheet.create({ | |
export default { | ||
name: "mock-widget", | ||
displayName: "Mock Widget", | ||
widget: MockWidget, | ||
widget: MockWidgetComponent, | ||
isLintable: true, | ||
// TODO(LEMS-2656): remove TS suppression | ||
// @ts-expect-error: Type 'UserInput' is not assignable to type 'MockWidget'. | ||
scorer: scoreMockWidget, | ||
} satisfies WidgetExports<typeof MockWidget>; | ||
// TODO(LEMS-2656): remove TS suppression | ||
// @ts-expect-error: Type 'UserInput' is not assignable to type 'PerseusMockWidgetUserInput'. | ||
validator: validateMockWidget, | ||
} satisfies WidgetExports<typeof MockWidgetComponent>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import validateMockWidget from "./validate-mock-widget"; | ||
|
||
import type {PerseusMockWidgetUserInput} from "./mock-widget-types"; | ||
|
||
describe("mock-widget", () => { | ||
it("should be invalid if no value provided", () => { | ||
const input: PerseusMockWidgetUserInput = {currentValue: ""}; | ||
|
||
const result = validateMockWidget(input); | ||
|
||
expect(result).toHaveInvalidInput(); | ||
}); | ||
|
||
it("should be valid if a value provided", () => { | ||
const input: PerseusMockWidgetUserInput = {currentValue: "a"}; | ||
|
||
const result = validateMockWidget(input); | ||
|
||
expect(result).toBeNull(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import type {PerseusMockWidgetUserInput} from "./mock-widget-types"; | ||
import type {ValidationResult} from "../../types"; | ||
|
||
function validateMockWidget( | ||
userInput: PerseusMockWidgetUserInput, | ||
): ValidationResult { | ||
if (userInput.currentValue == null || userInput.currentValue === "") { | ||
return { | ||
type: "invalid", | ||
message: "", | ||
}; | ||
} | ||
|
||
return null; | ||
} | ||
|
||
export default validateMockWidget; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.