Migrate PerseusWidgetsMap to use a merged interface to represent all available widgets #1936
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (5d5e415) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1936If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1936 |
|
Size Change: +788 B (+0.06%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
| * | ||
| * @see {@link https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation} | ||
| */ | ||
| export interface PerseusWidgetTypes { |
There was a problem hiding this comment.
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!
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
There was a problem hiding this comment.
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. :)
| * Our core set of Perseus widgets. | ||
| * | ||
| * This interface is the basis for "registering" all Perseus widget types. | ||
| * There should be one key/value pair for each supported widget. If you create | ||
| * a new widget, an entry should be added to this interface. | ||
| * | ||
| * Importantly, the key is not important and is not used anywhere outside of | ||
| * this interface so it is arbitrary. | ||
| * | ||
| * If you define the widget outside of this package, you can still add the new | ||
| * widget to this interface by writing the following in that pacakge that | ||
| * contains the widget. TypeScript will merge that definition of the | ||
| * `PerseusWidgets` with the one defined below. | ||
| * | ||
| * ```typescript | ||
| * declare module "@khanacademy/perseus" { | ||
| * interface PerseusWidgetTypes { | ||
| * Awesomeness: MyAwesomeNewWidget; | ||
| * } | ||
| * } | ||
| * | ||
| * type MyAwesomeNewWidget = WidgetOptions<'awesomeness', MyAwesomeNewWidgetOptions>; | ||
| * ``` | ||
| * | ||
| * This interface can be extended through the magic of TypeScript "Declaration | ||
| * merging". Specifically, we augment this module and extend this interface. | ||
| * | ||
| * @see {@link https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation} |
There was a problem hiding this comment.
Thanks for adding this docstring. I think it might also be worth mentioning that MyAwesomeNewWidget needs to be registered and what that looks like, but that might make more sense in the README.md.
| PerseusWidgetTypes, | ||
| MultiItem, | ||
| WidgetOptions, |
There was a problem hiding this comment.
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.
| * this interface so it is arbitrary. | ||
| * | ||
| * If you define the widget outside of this package, you can still add the new | ||
| * widget to this interface by writing the following in that pacakge that |
There was a problem hiding this comment.
| * widget to this interface by writing the following in that pacakge that | |
| * widget to this interface by writing the following in that package that |
| Radio: RadioWidget; | ||
| Sorter: SorterWidget; | ||
| Table: TableWidget; | ||
| Video: VideoWidget; |
There was a problem hiding this comment.
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?
| * | ||
| * @see {@link https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation} | ||
| */ | ||
| export interface PerseusWidgetTypes { |
There was a problem hiding this comment.
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.
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
widgetsprop ofPerseusRendereris 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 typecheckyarn testI'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! ✅