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.

Sign up for GitHub

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

Jump to bottom

feat(NOTIFY-1146): use better type system for user storage #4907

Merged
merged 5 commits into from
Nov 15, 2024
Merged

feat(NOTIFY-1146): use better type system for user storage #4907

merged 5 commits into from
Nov 15, 2024

Conversation

mathieuartu
Copy link
Contributor

@mathieuartu mathieuartu commented Nov 8, 2024 •
edited
Loading

Explanation

This PR adds a readonly object that stores feature names so we stop relying on string values for user storage operations.
This object is also now exported from the packages, in both SDK and UserStorage paths, so it can conveniently be used in external applications.

References

https://consensyssoftware.atlassian.net/browse/NOTIFY-1146

Fixes #4921

Changelog

@metamask/profile-sync-controller

  • CHANGED: Better type system for user storage, using and exporting a new USER_STORAGE_FEATURE_NAMES readonly object

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@mathieuartu mathieuartu added the team-notifications Notification Team changes. https://github.com/orgs/MetaMask/teams/notifications label Nov 8, 2024
@mathieuartu mathieuartu marked this pull request as ready for review November 8, 2024 13:43
@mathieuartu mathieuartu requested a review from a team as a code owner November 8, 2024 13:43
Prithpal-Sooriya
Prithpal-Sooriya reviewed Nov 8, 2024
View reviewed changes
packages/profile-sync-controller/src/shared/storage-schema.ts Outdated
Comment on lines 12 to 15
export enum UserStorageFeatureNames {
Notifications = 'notifications',
Accounts = 'accounts_v2',
Networks = 'networks',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to make 100% sure these names are identical to what they were before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I see that account_v2 change lol.

We might not need this as we are resolving the issue through a delete endpoint.

But not against it if we want a fresh clean slate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed that they are identical! :)
I'm leaning on the side of starting from scratch after having so many devs impacted, let's keep accounts_v2 AND add a safeguard deleting possible bogus entries. I'll have a PR ready probably tomorrow about this.

packages/profile-sync-controller/src/shared/storage-schema.ts
UserStorageSchema[Feature][0] extends typeof ALLOW_ARBITRARY_KEYS
? string
: UserStorageSchema[Feature][number];

type UserStorageFeatureAndKey = {
feature: UserStorageFeatures;
key: UserStorageFeatureKeys<UserStorageFeatures>;
feature: UserStorageFeatureNames;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice if some outside interfaces are backwards compatible. Supporting string unions or enums.

E.g. if I'm an SDK user, I now have to add an additional import for using user storage. That's not great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also can you make sure that we have the correct exports for the subpaths?

E.g. If I'm importing the SDK through /SDK, can I also access the enum from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a new update, making types retro compatible. Also ditched the enum for a plain old object!

So now every param could either use USER_STORAGE_FEATURE_NAMES.xxxx or 'notifications' | 'accounts_v2' | 'networks' | ...

@mathieuartu mathieuartu added the team-identity Identity Team changes. https://github.com/orgs/MetaMask/teams/identity label Nov 14, 2024
@mathieuartu mathieuartu requested a review from a team as a code owner November 14, 2024 21:51
@mathieuartu mathieuartu merged commit d699559 into main Nov 15, 2024
121 checks passed
@mathieuartu mathieuartu deleted the feat/better_type_system_user_storage branch November 15, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Prithpal-Sooriya Prithpal-Sooriya Prithpal-Sooriya approved these changes

Assignees
No one assigned
Labels
team-identity Identity Team changes. https://github.com/orgs/MetaMask/teams/identity team-notifications Notification Team changes. https://github.com/orgs/MetaMask/teams/notifications
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[UserStorageController] Improve type safety for user storage schema
2 participants
@mathieuartu @Prithpal-Sooriya