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

Remove NotificationController #4815

Merged
merged 7 commits into from
Nov 29, 2024
Merged

Conversation

hmalik88
Copy link
Contributor

Removes the NotificationController in favor of NotificationServicesController, see #4809

@hmalik88 hmalik88 requested review from a team as code owners October 17, 2024 21:09
Copy link

socket-security bot commented Oct 17, 2024

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/@metamask/[email protected]

View full report↗︎

MajorLift
MajorLift previously approved these changes Oct 18, 2024
Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

LGTM!

@Prithpal-Sooriya
Copy link
Contributor

RIP 🥲

@Prithpal-Sooriya
Copy link
Contributor

The @MetaMask/notifications team will align with the @MetaMask/snaps team on this.

Our notifications team were preparing the "notification unification" in Q1 (unify snaps, confirmations, our back-end notifications, and future notifications or alerts), but, as this seems to be moving forward, we can expedite it 😅.

We can have a brainstorm and discussion on how we can create a unified/orchestrated notification controller that:

  1. Allows other teams to manage and own their own notifications. Make it easy to contribute and extend.
  2. Create a clean/minimal interface that reflects 1:1 with the UI
  3. Prevent god controllers (e.g. a notification controller that becomes too large and hard to manage, our team is already starting to feel this as we are planning to release more and more notifications).

This can be done in iterative steps. I'm just bringing this up as the current NotificationServicesController is very large, very complex, has confusing method names, and is getting harder and harder to extend.

Copy link
Member

@FrederikBolding FrederikBolding left a comment

Choose a reason for hiding this comment

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

RIP🫡

@hmalik88 hmalik88 merged commit 60e3ce7 into main Nov 29, 2024
120 checks passed
@hmalik88 hmalik88 deleted the hm/remove-notification-controller branch November 29, 2024 11:20
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