Don't set default apiOptions - they're provided always#3851
Conversation
|
Size Change: -15 B (0%) Total Size: 517 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (c828848) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3851If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3851If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3851 |
| import type {PropsFor} from "@khanacademy/wonder-blocks-core"; | ||
| import type {UserEvent} from "@testing-library/user-event"; | ||
|
|
||
| const HarnessedEditor = React.forwardRef< |
There was a problem hiding this comment.
I opted to create a "harnessed editor" which has the onChange and (now required) apiOptions defaulted. This allows us to render the editor in each test without a bunch of duplication, but also allows providing onChange where needed.
This pattern is now duplicated across three editors: expression-editor, free-response-editor, and numeric-input-editor... so that could get deduped eventually.
| }); | ||
|
|
||
| it("should render", async () => { | ||
| render(<ExpressionEditor onChange={() => undefined} />); |
There was a problem hiding this comment.
I think I like that we don't provide the prop when it's irrelevant to the test.
|
|
||
| static defaultProps = { | ||
| ...expressionLogic.defaultWidgetOptions, | ||
| apiOptions: ApiOptions.defaults, |
There was a problem hiding this comment.
This is the core of the fix. Providing a default apiOptions causes them to get "smuggled" into the widget options that the preview system then tries to serialize across to the preview via postMessage().
jeremywiebe
force-pushed
the
jer/bugfix-editor-crash-adding-widget
branch
from
8c6219e to
c828848
Compare
catandthemachines
left a comment
catandthemachines
left a comment
There was a problem hiding this comment.
This all looks great. Thank you Jeremy!
…- they're provided always
…tions from defaultProps in ExpressionEditor, NumericInputEditor, and FreeResponseEditor. Its provided by the widget editor wrapper always.
jeremywiebe
force-pushed
the
jer/bugfix-editor-crash-adding-widget
branch
from
c828848 to
5912789
Compare
2 participants
Summary:
Adding a widget to the editor was crashing the preview with a
DataCloneErrorbecausepostMessagecan't serialize functions. The crash traced back to three widget editors —ExpressionEditor,NumericInputEditor, andFreeResponseEditor— each includingapiOptions: ApiOptions.defaultsin their staticdefaultProps.ApiOptions.defaultscontains fields that cannot be passed through the structured-clone algorithm used bypostMessagefor sending data to the preview iframe.The bug surfaced because
Editor._addWidgetToContentuseswidgetEditor.defaultPropsas the initial options for a newly added widget. That put the unserializable values directly into the widget data, which then flowed throughpostMessageto the preview iframe.The fix is to remove
apiOptionsfrom those three editors'defaultProps. Widget editors should never provide a defaultapiOptions— theWidgetEditorwrapper always supplies it as a prop, so having it indefaultPropswas both redundant and harmful.Widget editor tests have been also updated to handle
apiOptionsbeing required and not defaulted now.Issue: LEMS-3741
Test plan:
EditorPagedemo in Storybook, add anexpressionwidget to the content, and confirm the preview no longer crashes with aDataCloneErrorpnpm test