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

refactor(app): refine useNotifyService #15093

Merged
merged 1 commit into from
May 6, 2024
Merged

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented May 6, 2024

Overview

useNotifyService leaks refetch frequencies unnecessarily. We can refactor this logic to useNotifyService where it belongs and tighten the abstraction.

Test Plan

  • Verified notifications work.

Risk assessment

low

useNotifyService leaks refetch frequencies unnecessarily. We can refactor this logic to
useNotifyService.
@mjhuff mjhuff requested review from a team May 6, 2024 12:32
@mjhuff mjhuff requested review from a team as code owners May 6, 2024 12:32
@mjhuff mjhuff requested review from koji and removed request for a team May 6, 2024 12:32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDK how this test was passing before, but it correctly broke now. I've mocked it in the expected way, and it passes.

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Nice!

@mjhuff mjhuff merged commit d1278cd into edge May 6, 2024
26 checks passed
@mjhuff mjhuff deleted the app_refine-notify-service branch May 6, 2024 15:09
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
useNotifyService leaks refetch frequencies unnecessarily. We can refactor this logic to useNotifyService where it belongs and tighten the abstraction.
Carlos-fernandez pushed a commit that referenced this pull request Jun 3, 2024
useNotifyService leaks refetch frequencies unnecessarily. We can refactor this logic to useNotifyService where it belongs and tighten the abstraction.
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.

3 participants