Migrate PerseusWidgetsMap to use a merged interface to represent all available widgets #1936

jeremywiebe · 2024-11-29T21:36:41Z

Summary:

Perseus is designed to be able to support widgets registered into it at runtime. These widgets can be built in a separate npm package or even in the hosting application. As a result, the way that we've defined the widgets prop of PerseusRenderer is too rigid and poses long-term maintenance issues.

As I was working on slitting out the bits of validation from scoring that we'll still be able to do on the client-side, I faced the same challenge (we will have a map of widget IDs for validation).

Working with Kevin Barabash, we figured out a way to define an interface that represents all registered widgets and then use some TypeScript types to generate the PerseusWidgetsMap. This allows us to extend the "known set" of widgets from outside of Perseus.

Although this is not an immediate concern, it will become more and more troublesome to build maps of widget information that are hard-coded to our current set of widgets.

Issue: "none"

Test plan:

yarn typecheck
yarn test

I'll import this npm snapshot into webapp to check that all types are fine there also.

EDIT: I installed the snapshot package in webapp and it type checks! ✅

…available widgets

github-actions · 2024-11-29T21:39:21Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1936

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

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

github-actions · 2024-11-29T21:39:44Z

Size Change: +918 B (+0.07%)

Total Size: 1.29 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	425 kB	+918 B (+0.22%)

ℹ️ 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`	697 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.7 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-11-29T21:39:53Z

packages/perseus/src/perseus-types.ts

+ *
+ * @see {@link https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation}
+ */
+export interface PerseusWidgetTypes {


Although this looks like the same hard-coded list of widgets as PerseusWidgetsMap is in the "before" state of this PR, the magic is that we can extend this interface from anywhere (even in our own Perseus unit tests if we want to work with a mock widget). Once we've added an entry to this interface, PerseusWidgetsMap automatically know's about that widget!

khan-actions-bot · 2024-11-29T21:47:09Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/tender-planets-greet.md, packages/perseus/src/index.ts, packages/perseus/src/perseus-types.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

SonicScrewdriver

This is WAY better than our first attempt, thank you greatly for this work! This seems considerably more maintainable and readable. I'm not seeing anything that should cause any issues with the Extract Perseus Data logic, and if there's no type errors, this gets the all clear from me.

Thank you for adding some truly excellent comments as well. :)

kevinb-khan

LGTM. Thanks for doing the due diligence to verify that this plan will work. I left a few documentation suggestions in the comments – nothing blocking. 🚢

packages/perseus/src/perseus-types.ts

catandthemachines

Looks like a great improvement! Thanks Jeremy!!

handeyeco

Cool, thanks for digging into this more!

handeyeco · 2024-12-03T15:09:40Z

packages/perseus/src/index.ts

+    PerseusWidgetTypes,
    MultiItem,
+    WidgetOptions,


Do you know that these will be used in Webapp or are we preemptively exporting them? If not, I wonder if it would be better to keep them internal until we have a use for them externally.

PerseusWidgetTypes needs to be exported so that it can be extended from anywhere outside of Perseus. @kevinb-khan has already had a use case for this while building a hackathon widget inside of webapp.

WidgetOptions is a very useful helper type for defining a widget's widget options structure.

Both are forward-looking, but it seemed reasonable to export them as part of this PR given the "widgets can be created outside of the @khanacademy/perseus" package nature.

packages/perseus/src/perseus-types.ts

handeyeco · 2024-12-03T15:12:40Z

packages/perseus/src/perseus-types.ts

+    Radio: RadioWidget;
+    Sorter: SorterWidget;
+    Table: TableWidget;
+    Video: VideoWidget;


Not related to your PR and no action needed: can we leverage this or something like this to get rid of basic-widgets, extra-widgets, and all-widgets?

I don't think so. This interface is a registry of widget option types whereas the things you mentioned are the "runtime registries". We could certainly merge all those three items together though.

handeyeco · 2024-12-03T15:16:05Z

packages/perseus/src/perseus-types.ts

+ *
+ * @see {@link https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation}
+ */
+export interface PerseusWidgetTypes {


I think this is a step in the right direction, I just wish we could get to a place where the Renderer-layer of Perseus is completely unaware of the Widget-layer data which would mean finding ways to eliminate these type of widget union/map things.

Agreed! I've spent a decent amount of time thinking about how we could make the lower level types opaque to the Renderers, I don't have a solution yet.

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

@Myranae

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/[email protected] ### Minor Changes - [#1901](#1901) [`051c50cf7`](051c50c) Thanks [@Myranae](https://github.com/Myranae)! - Introduces a validation function for the number line widget (extracted from the scoring function). - [#1936](#1936) [`d05272661`](d052726) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Changes the PerseusWidgetsMap to be extensible so that widgets can be registered outside of Perseus and still have full type safety. - [#1832](#1832) [`e3062b3c8`](e3062b3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - [Numeric Input] - Update the UI to match Expression widget ### Patch Changes - [#1937](#1937) [`3cdabf1a3`](3cdabf1) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - TypeScript fixes - [#1948](#1948) [`e21a3a39b`](e21a3a3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Refactor internally used object mapping utilities to use ES6 exports - [#1938](#1938) [`5e8d8468b`](5e8d846) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Type fixes - [#1942](#1942) [`1d2b4e7bf`](1d2b4e7) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Ensure that zoomed-in images retain alt text - [#1861](#1861) [`763a4ba38`](763a4ba) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - [Numeric Input] - Show format options as a list - [#1947](#1947) [`b8926e38a`](b8926e3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Minor refactoring of ServerItemRenderer's componentDidUpdate to reduce duplication - [#1946](#1946) [`f35512786`](f355127) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Refactor scoring for `group` widget to follow the same pattern as all other widgets - [#1891](#1891) [`ef819ea95`](ef819ea) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - [Numeric Input] - Associate format options tooltip content with input field for assistive technologies - [#1945](#1945) [`e69ca3146`](e69ca31) Thanks [@nishasy](https://github.com/nishasy)! - Add global styles to reflect prod styling - [#1923](#1923) [`be8c06c75`](be8c06c) Thanks [@benchristel](https://github.com/benchristel)! - Internal: convert backgroundImage dimensions to numbers during parsing. - [#1934](#1934) [`129adebef`](129adeb) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Improve comments on some Perseus types - [#1924](#1924) [`2d89ef87d`](2d89ef8) Thanks [@benchristel](https://github.com/benchristel)! - Internal: add and pass regression tests for PerseusItem parser's handling of legacy data ## @khanacademy/[email protected] ### Patch Changes - [#1938](#1938) [`5e8d8468b`](5e8d846) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Type fixes - [#1937](#1937) [`3cdabf1a3`](3cdabf1) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Ensure links opening to style guide (Google Docs) set `rel="noreferrer"` - [#1946](#1946) [`f35512786`](f355127) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Remove debugging call in GraphSettings component - Updated dependencies \[[`3cdabf1a3`](3cdabf1), [`e21a3a39b`](e21a3a3), [`5e8d8468b`](5e8d846), [`1d2b4e7bf`](1d2b4e7), [`763a4ba38`](763a4ba), [`b8926e38a`](b8926e3), [`f35512786`](f355127), [`ef819ea95`](ef819ea), [`e69ca3146`](e69ca31), [`be8c06c75`](be8c06c), [`051c50cf7`](051c50c), [`129adebef`](129adeb), [`d05272661`](d052726), [`2d89ef87d`](2d89ef8), [`e3062b3c8`](e3062b3)]: - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: mark-fitzgerald Required Reviewers: Approved By: mark-fitzgerald Checks: ⏭️ Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1944

jeremywiebe added 2 commits November 29, 2024 13:10

Migrate PerseusWidgetsMap to use a merged interface to represent all …

b8eb8cd

…available widgets

Changeset

80721e7

jeremywiebe self-assigned this Nov 29, 2024

jeremywiebe commented Nov 29, 2024

View reviewed changes

jeremywiebe marked this pull request as ready for review November 29, 2024 21:46

khan-actions-bot requested a review from a team November 29, 2024 21:47

jeremywiebe requested review from benchristel, SonicScrewdriver, handeyeco and kevinb-khan November 29, 2024 21:47

SonicScrewdriver approved these changes Nov 29, 2024

View reviewed changes

kevinb-khan approved these changes Nov 30, 2024

View reviewed changes

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

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

catandthemachines approved these changes Dec 2, 2024

View reviewed changes

jeremywiebe added 2 commits December 2, 2024 13:40

Add note about widget id format

ee5b07f

Add note that you still need to register widgets

5d5e415

handeyeco approved these changes Dec 3, 2024

View reviewed changes

jeremywiebe and others added 2 commits December 3, 2024 11:51

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

03ee194

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

Merge branch 'main' into jer/perseus-widget-map-extensible

6a590e7

jeremywiebe merged commit d052726 into main Dec 4, 2024
9 checks passed

jeremywiebe deleted the jer/perseus-widget-map-extensible branch December 4, 2024 02:22

khan-actions-bot mentioned this pull request Dec 4, 2024

Version Packages #1944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate PerseusWidgetsMap to use a merged interface to represent all available widgets #1936

Migrate PerseusWidgetsMap to use a merged interface to represent all available widgets #1936

jeremywiebe commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024 •

edited

Loading

jeremywiebe Nov 29, 2024

khan-actions-bot commented Nov 29, 2024

SonicScrewdriver left a comment •

edited

Loading

kevinb-khan left a comment

catandthemachines left a comment

handeyeco left a comment

handeyeco Dec 3, 2024

jeremywiebe Dec 4, 2024

handeyeco Dec 3, 2024

jeremywiebe Dec 4, 2024

handeyeco Dec 3, 2024

jeremywiebe Dec 4, 2024

Migrate PerseusWidgetsMap to use a merged interface to represent all available widgets #1936

Migrate PerseusWidgetsMap to use a merged interface to represent all available widgets #1936

Conversation

jeremywiebe commented Nov 29, 2024 • edited Loading

Summary:

Test plan:

github-actions bot commented Nov 29, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Nov 29, 2024 • edited Loading

jeremywiebe Nov 29, 2024

Choose a reason for hiding this comment

khan-actions-bot commented Nov 29, 2024

Gerald

SonicScrewdriver left a comment • edited Loading

Choose a reason for hiding this comment

kevinb-khan left a comment

Choose a reason for hiding this comment

catandthemachines left a comment

Choose a reason for hiding this comment

handeyeco left a comment

Choose a reason for hiding this comment

handeyeco Dec 3, 2024

Choose a reason for hiding this comment

jeremywiebe Dec 4, 2024

Choose a reason for hiding this comment

handeyeco Dec 3, 2024

Choose a reason for hiding this comment

jeremywiebe Dec 4, 2024

Choose a reason for hiding this comment

handeyeco Dec 3, 2024

Choose a reason for hiding this comment

jeremywiebe Dec 4, 2024

Choose a reason for hiding this comment

jeremywiebe commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024 •

edited

Loading

SonicScrewdriver left a comment •

edited

Loading