-
Notifications
You must be signed in to change notification settings - Fork 351
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
Deprecate metadata
in renderer, hint, and Group widget data schemas
#2242
Conversation
…up widget data schemas
…er, hint, and Group widget data schemas
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (8162367) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2242 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2242 |
Size Change: -105 B (-0.01%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
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.
Nothing blocking on my end. Looks like a very promising change.
return _.extend({}, this.editor.current?.serialize(), { | ||
metadata: this.props.metadata, | ||
}); | ||
return _.extend({}, this.editor.current?.serialize()); |
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.
Could this be simplified?
return _.extend({}, this.editor.current?.serialize()); | |
return this.editor.current?.serialize(); |
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.
Yeah, I was being cautious by not messing with it. Given that it's making a copy and I don't trust the editors not to mutate objects, I'd prefer:
return {...this.editor.current?.serialize()};
/** | ||
* @deprecated - metadata is no longer used by the Group widget | ||
*/ |
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.
We're not able to just get rid of it?
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.
That would be a breaking change — callers of ServerItemRenderer
would get errors if they passed a GroupMetadataEditor
(and webapp does).
Given the difficulties of release coordination with AX, I'd prefer to avoid breaking changes if at all possible.
// deprecated | ||
metadata: any, |
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.
We can't just ignore it? It has to be parsed?
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.
This is probably way too paranoid, but I want to avoid the situation where
- we delete
metadata
from the schema and parser. - 5 years from now, someone decides it would be cool if
PerseusRenderer
had ametadata
field. - they get errors in production because legacy content already has
metadata
, and it's shaped totally differently from whatever new metadata format they came up with.
I guess the parser regression tests should stop them before they get to step 3, though?
.changeset/tall-llamas-crash.md
Outdated
@@ -0,0 +1,7 @@ | |||
--- | |||
"@khanacademy/perseus": minor | |||
"@khanacademy/perseus-core": patch |
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.
Isn't this a breaking change in perseus-core
? If someone had defined a PerseusRenderer
with metadata
and upgraded perseus-core
they would get a TS error and would be required to change their code?
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.
Yeah, I guess that's true!
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.
I've deprecated metadata
rather than removing it outright, and changed all of these version bumps to minor
.
I've confirmed that Group widgets can still be rendered and edited after this change. |
metadata
in renderer, hint, and Group widget data schemas
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@54.1.0 ### Minor Changes - [#2242](#2242) [`e187c6b67`](e187c6b) Thanks [@benchristel](https://github.com/benchristel)! - Deprecate the `metadata` field in renderer, hint, and Group widget data schemas. - [#2156](#2156) [`cbd5a6528`](cbd5a65) Thanks [@Myranae](https://github.com/Myranae)! - Implement a widget export function to filter out rubric data from widget options for the Matcher widget ### Patch Changes - [#2215](#2215) [`62ed407b8`](62ed407) Thanks [@Myranae](https://github.com/Myranae)! - Update Sorter's public widget option function to use Math.random and shuffle - Updated dependencies \[[`bae77a63c`](bae77a6), [`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-linter@1.2.18 - @khanacademy/perseus-core@3.7.0 - @khanacademy/kas@0.4.16 - @khanacademy/keypad-context@1.0.19 - @khanacademy/kmath@0.3.5 - @khanacademy/math-input@22.2.6 - @khanacademy/perseus-score@2.2.2 - @khanacademy/pure-markdown@0.3.27 - @khanacademy/simple-markdown@0.13.20 ## @khanacademy/perseus-core@3.7.0 ### Minor Changes - [#2246](#2246) [`e63f83d0d`](e63f83d) Thanks [@benchristel](https://github.com/benchristel)! - Update `parseAndMigratePerseusItem` and `parseAndMigratePerseusArticle` to accept legacy data formats observed in production - [#2242](#2242) [`e187c6b67`](e187c6b) Thanks [@benchristel](https://github.com/benchristel)! - Deprecate the `metadata` field in renderer, hint, and Group widget data schemas. - [#2215](#2215) [`62ed407b8`](62ed407) Thanks [@Myranae](https://github.com/Myranae)! - Update Sorter's public widget option function to use Math.random and shuffle - [#2156](#2156) [`cbd5a6528`](cbd5a65) Thanks [@Myranae](https://github.com/Myranae)! - Implement a widget export function to filter out rubric data from widget options for the Matcher widget ## @khanacademy/perseus-editor@17.7.0 ### Minor Changes - [#2242](#2242) [`e187c6b67`](e187c6b) Thanks [@benchristel](https://github.com/benchristel)! - Deprecate the `metadata` field in renderer, hint, and Group widget data schemas. ### Patch Changes - Updated dependencies \[[`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-core@3.7.0 - @khanacademy/perseus@54.1.0 - @khanacademy/kas@0.4.16 - @khanacademy/keypad-context@1.0.19 - @khanacademy/kmath@0.3.5 - @khanacademy/math-input@22.2.6 - @khanacademy/perseus-score@2.2.2 - @khanacademy/pure-markdown@0.3.27 ## @khanacademy/kas@0.4.16 ### Patch Changes - Updated dependencies \[[`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-core@3.7.0 ## @khanacademy/keypad-context@1.0.19 ### Patch Changes - Updated dependencies \[[`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-core@3.7.0 ## @khanacademy/kmath@0.3.5 ### Patch Changes - Updated dependencies \[[`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-core@3.7.0 ## @khanacademy/math-input@22.2.6 ### Patch Changes - Updated dependencies \[[`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-core@3.7.0 - @khanacademy/keypad-context@1.0.19 ## @khanacademy/perseus-linter@1.2.18 ### Patch Changes - [#2240](#2240) [`bae77a63c`](bae77a6) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - [Linter] Remove Math Font Size rule from editor linter - Updated dependencies \[[`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-core@3.7.0 ## @khanacademy/perseus-score@2.2.2 ### Patch Changes - Updated dependencies \[[`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-core@3.7.0 - @khanacademy/kas@0.4.16 - @khanacademy/kmath@0.3.5 ## @khanacademy/pure-markdown@0.3.27 ### Patch Changes - Updated dependencies \[[`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-core@3.7.0 - @khanacademy/simple-markdown@0.13.20 ## @khanacademy/simple-markdown@0.13.20 ### Patch Changes - Updated dependencies \[[`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-core@3.7.0 ## @khanacademy/perseus-dev-ui@5.2.2 ### Patch Changes - Updated dependencies \[[`bae77a63c`](bae77a6), [`e63f83d0d`](e63f83d), [`e187c6b67`](e187c6b), [`62ed407b8`](62ed407), [`cbd5a6528`](cbd5a65)]: - @khanacademy/perseus-linter@1.2.18 - @khanacademy/perseus-core@3.7.0 - @khanacademy/kas@0.4.16 - @khanacademy/kmath@0.3.5 - @khanacademy/math-input@22.2.6 - @khanacademy/pure-markdown@0.3.27 - @khanacademy/simple-markdown@0.13.20
We aren't using
metadata
. As near as I can tell, it was added in 2014 so contentcreators could add "tags" to content, but I don't know why it was added or if it was
ever used.
We are seeing Perseus JSON parser errors in webapp because
metadata
is null in someexercises. This PR fixes the errors by changing the type of
metadata
toany
, markingit deprecated, and removing references to it.
Issue: none
Test plan: