-
-
Notifications
You must be signed in to change notification settings - Fork 255
feat: Make breaking changes to next base-controller metadata
#6593
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
Conversation
Make the planned breaking changes to controller metadata. This includes renaming `anonymous` to `includeInDebugSnapshot`, and it includes making the two new metadata properties required. Relates to #6443
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.
Bug: Metadata Constraint Mismatch
The StatePropertyMetadataConstraint type is out of sync with StatePropertyMetadata. It still uses the anonymous property instead of includeInDebugSnapshot, and includeInStateLogs and usedInUi are optional instead of required. This breaks its intended supertype relationship, causing type incompatibility.
packages/base-controller/src/next/BaseController.ts#L109-L120
core/packages/base-controller/src/next/BaseController.ts
Lines 109 to 120 in 1474ccc
| /** | |
| * A universal supertype of `StatePropertyMetadata` types. | |
| * This type can be assigned to any `StatePropertyMetadata` type. | |
| */ | |
| export type StatePropertyMetadataConstraint = { | |
| anonymous: boolean | StateDeriverConstraint; | |
| includeInStateLogs?: boolean | StateDeriverConstraint; | |
| persist: boolean | StateDeriverConstraint; | |
| usedInUi?: boolean; | |
| }; | |
cryptodev-2s
left a comment
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.
LGTM!
## Explanation In the `next` export of `base-controller`, the type `StatePropertyMetadataConstraint` mistakenly referenced the old metadata property name `anonymous` rather than `includeInDebugSnapshot`. The type has been updated to use the correct name. ## References Fixes a mistake introduced in #6593 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Corrects `StatePropertyMetadataConstraint` in the experimental `next` export to use `includeInDebugSnapshot` instead of `anonymous`, and updates the changelog. > > - **Type fix (experimental `next`)** > - Update `StatePropertyMetadataConstraint` in `packages/base-controller/src/next/BaseController.ts` to use `includeInDebugSnapshot` (replacing `anonymous`). > - **Changelog** > - Add Unreleased entry documenting the fix in `packages/base-controller/CHANGELOG.md`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 31f5c11. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Explanation
Make the planned breaking changes to controller metadata. This includes renaming
anonymoustoincludeInDebugSnapshot, and it includes making the two new metadata properties required.References
Relates to #6443
Checklist