Skip to content
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

Move enum logic from Webapp to Perseus #1058

Merged
merged 3 commits into from
Mar 18, 2024
Merged

Move enum logic from Webapp to Perseus #1058

merged 3 commits into from
Mar 18, 2024

Conversation

handeyeco
Copy link
Contributor

@handeyeco handeyeco commented Mar 6, 2024

Summary:

In this PR I added some logic in Webapp knowing I would need to move it to Perseus; now that I'm trying to add the CEDAR event to Mobile, the time has come.


I didn't get any ideas on how to implement the below and I need to move on, so I'm going to move forward with the original plan.

I'm wondering if this could be done in a cleaner way. In each widget we have an export; for example this is Radio's:

export default {
    name: "radio", // <- type: string
    displayName: "Radio / Multiple choice",
    accessible: true,
    widget: Radio,
    transform: transform,
    staticTransform: transform,
    version: {major: 1, minor: 0},
    propUpgrades: propUpgrades,
    isLintable: true,
} as WidgetExports<typeof Radio>;

We also do something like this in perseus-types.ts:

export type RadioWidget = Widget<'radio', PerseusRadioWidgetOptions>;

type Widget<Type extends string, Options> = {
    // The "type" of widget which will define what the Options field looks like
    type: Type;

    // ...etc...
};

Then:

export type PerseusWidgetsMap = {
    // ...etc...
} & {
    [key in `radio ${number}`]: RadioWidget;
} & {
    // ...etc...

It feels like we're having to maintain a lot of places where there are lists of widgets; now this would add even more. Is there a way to derive this type information from the widgets themselves? For instance:

// in each widget file
export default {
    name: "radio",
    enum: "RADIO",
    // ...etc...
} as WidgetExports<typeof Radio>;

// in a shared file
const perseusWidgets = [
    RadioWidget,
    LightPuzzleWidget,
    // ...etc...
]

// Could theses be derived types?
const PerseusWidgetNames = perseusWidgets.map(w => w.name) as const;
const PerseusWidgetEnums = perseusWidgets.map(w => w.enum) as const;

That way the widgets themselves are responsible for providing their own name and enum values? And we have one master list of widgets that is used to derive other lists?

Issue: MOB-5441

@handeyeco handeyeco self-assigned this Mar 6, 2024
@handeyeco handeyeco changed the title move enum logic from Webapp to Perseus WIP: move enum logic from Webapp to Perseus Mar 6, 2024
Copy link
Contributor

github-actions bot commented Mar 6, 2024

Size Change: +863 B (0%)

Total Size: 811 kB

Filename Size Change
packages/math-input/dist/es/index.js 80.4 kB +155 B (0%)
packages/perseus-editor/dist/es/index.js 262 kB -175 B (0%)
packages/perseus/dist/es/index.js 386 kB +883 B (0%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.1 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/perseus-core/dist/es/index.js 908 B
packages/perseus-error/dist/es/index.js 878 B
packages/perseus-linter/dist/es/index.js 21.8 kB
packages/pure-markdown/dist/es/index.js 3.68 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 85.98726% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 67.25%. Comparing base (547eb28) to head (0a98447).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1058      +/-   ##
==========================================
+ Coverage   65.12%   67.25%   +2.12%     
==========================================
  Files         432      436       +4     
  Lines       96215    96445     +230     
  Branches     6544    10110    +3566     
==========================================
+ Hits        62664    64867    +2203     
+ Misses      33551    31578    -1973     

Impacted file tree graph

Files Coverage Δ
packages/perseus/src/index.ts 100.00% <100.00%> (ø)
packages/perseus/src/util/widget-enum-utils.ts 85.62% <85.62%> (ø)

... and 108 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 547eb28...0a98447. Read the comment docs.

explanation: "EXPLANATION",
expression: "EXPRESSION",
"graded-group-set": "GRADED_GROUP",
"graded-group": "GRADED_GROUP_SET",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these reversed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's weird. That's copy/pasted from prod webapp which means it's live on the site. 🤔 Not that I think we care all that much about graded-group statistics (compared to more interactive widgets).

@handeyeco handeyeco marked this pull request as ready for review March 6, 2024 21:33
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Mar 6, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/silver-humans-sell.md, packages/perseus/src/index.ts, packages/perseus/src/util/widget-enum-utils.ts

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

Copy link
Contributor

github-actions bot commented Mar 6, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1058

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

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

@handeyeco handeyeco changed the title WIP: move enum logic from Webapp to Perseus Move enum logic from Webapp to Perseus Mar 11, 2024
@handeyeco handeyeco requested a review from a team March 11, 2024 14:24

return widgetEnum;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh lovely this can replace that function in the coach report utils! :)

Copy link
Contributor

@SonicScrewdriver SonicScrewdriver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful, thank you!

@handeyeco handeyeco merged commit a431883 into main Mar 18, 2024
39 checks passed
@handeyeco handeyeco deleted the MOB-5441/widget-enum branch March 18, 2024 19:45
SonicScrewdriver added a commit that referenced this pull request Mar 20, 2024
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/perseus@20.6.0

### Minor Changes

- [#1090](#1090)
[`20a23a39`](20a23a3)
Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Added
new API methods to support our Coach Report Response view.


- [#1058](#1058)
[`a431883d`](a431883)
Thanks [@handeyeco](https://github.com/handeyeco)! - Move widget enum
helpers from Webapp to Perseus

### Patch Changes

- [#1095](#1095)
[`11e04962`](11e0496)
Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Tooling:
Upgrade projects to use TypeScript v5.4.2


- [#1093](#1093)
[`0cd66f88`](0cd66f8)
Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Change
APIOptions and APIOptionsWithDefaults comments to JSDoc comments for
better tooling support


- [#1088](#1088)
[`47eade13`](47eade1)
Thanks [@benchristel](https://github.com/benchristel)! - Refactor the
InteractiveGraph component to avoid mounting the legacy interactive
graph when Mafs is enabled. This ensures that state from the legacy
graph doesn't interfere with the Mafs graph

- Updated dependencies
\[[`11e04962`](11e0496)]:
    -   @khanacademy/simple-markdown@0.11.4
    -   @khanacademy/pure-markdown@0.3.1

## @khanacademy/perseus-dev-ui@1.3.3

### Patch Changes

- Updated dependencies
\[[`11e04962`](11e0496)]:
    -   @khanacademy/simple-markdown@0.11.4
    -   @khanacademy/pure-markdown@0.3.1

## @khanacademy/perseus-editor@5.2.1

### Patch Changes

- Updated dependencies
\[[`20a23a39`](20a23a3),
[`11e04962`](11e0496),
[`a431883d`](a431883),
[`0cd66f88`](0cd66f8),
[`47eade13`](47eade1)]:
    -   @khanacademy/perseus@20.6.0

## @khanacademy/pure-markdown@0.3.1

### Patch Changes

- Updated dependencies
\[[`11e04962`](11e0496)]:
    -   @khanacademy/simple-markdown@0.11.4

## @khanacademy/simple-markdown@0.11.4

### Patch Changes

- [#1095](#1095)
[`11e04962`](11e0496)
Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Tooling:
Upgrade projects to use TypeScript v5.4.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants