Skip to content

Conversation

@Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Dec 15, 2025

Explanation

The current state includes ESLint suppressions for the @typescript-eslint/explicit-function-return-type rule across several files in the notification-services-controller package. These suppressions hinder type safety and code clarity.

This PR addresses these issues by:

  1. Adding explicit return types to all functions previously suppressed for @typescript-eslint/explicit-function-return-type in the following files:
    • packages/notification-services-controller/src/NotificationServicesPushController/mocks/mockResponse.ts
    • packages/notification-services-controller/src/NotificationServicesPushController/services/endpoints.ts
    • packages/notification-services-controller/src/NotificationServicesPushController/services/services.test.ts
  2. Removing the corresponding entries from eslint-suppressions.json.

For services.test.ts, complex return types for test helper functions (arrangeMocks) were defined inline to ensure accuracy and avoid any. Additionally, import type nock from 'nock' was added to correctly type nock.Scope instances.

References

  • Fixes ASSETS-2100

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 communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Open in Cursor Open in Web


Note

Adds explicit function return types in push controller mocks, endpoints, and tests, and removes corresponding ESLint suppressions.

  • Notification Services Push Controller:
    • mocks/mockResponse.ts: Add explicit MockResponse return types to mock response factories.
    • services/endpoints.ts: Add explicit string return types to PUSH_API and REGISTRATION_TOKENS_ENDPOINT.
    • services/services.test.ts: Add explicit return types to test helpers (e.g., mockErrorLog, act); keep some helpers inferred with targeted ESLint disable comments.
  • Tooling:
    • eslint-suppressions.json: Remove entries for @typescript-eslint/explicit-function-return-type related to the above files.

Written by Cursor Bugbot for commit 43b5233. This will update automatically on new commits. Configure here.

@cursor
Copy link

cursor bot commented Dec 15, 2025

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@Prithpal-Sooriya
Copy link
Contributor Author

@cursor update the packages/notification-services-controller/CHANGELOG.md changelog to reflect changes made.

@cursor
Copy link

cursor bot commented Dec 15, 2025

I've updated the packages/notification-services-controller/CHANGELOG.md to reflect the changes made in this PR. The new entry has been added under the ## [Unreleased] section:

### Changed

- Add explicit return types to functions in `NotificationServicesPushController` ([#7480](https://github.com/MetaMask/core/pull/7480))

This entry describes the ESLint cleanup work done in this PR, which adds explicit return types to functions in the following files:

  • mocks/mockResponse.ts
  • services/endpoints.ts
  • services/services.test.ts

Open in Cursor Open in Web

Learn more about Cursor Agents

const arrangeMocks = (override?: {
mockPut?: { status: number };
}): {
params: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cursor I'm not a fan of the return type eslint here (it is pretty loose).
Lets disable for this line and the reason is because this is an internal testing util

Copy link

@cursor cursor bot Dec 15, 2025

Choose a reason for hiding this comment

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

Done! I've updated both arrangeMocks functions in services.test.ts to use eslint-disable comments instead of the verbose inline return types:

// Internal testing utility - return type is inferred
// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
const arrangeMocks = (override?: { mockPut?: { status: number } }) => {

The changes:

  1. Replaced the verbose inline return types with eslint-disable-next-line comments
  2. Added explanation comments: "Internal testing utility - return type is inferred"
  3. Removed the now-unused import type nock from 'nock'

All tests pass and ESLint is happy with the changes.


Open in Cursor Open in Web

Learn more about Cursor Agents

@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review December 15, 2025 11:12
@Prithpal-Sooriya Prithpal-Sooriya requested review from a team as code owners December 15, 2025 11:12
@Prithpal-Sooriya Prithpal-Sooriya changed the title Notification services eslint cleanup chore: Notification services eslint cleanup Dec 15, 2025
juanmigdr
juanmigdr previously approved these changes Dec 15, 2025
salimtb
salimtb previously approved these changes Dec 15, 2025
@Prithpal-Sooriya
Copy link
Contributor Author

@cursor please remove the changelog changes, as these eslint changes does not impact the implementation

@cursor
Copy link

cursor bot commented Dec 15, 2025

Done! I've removed the changelog entry for this PR. Since ESLint cleanup changes don't impact the implementation, they don't need to be documented in the changelog.


Open in Cursor Open in Web

Learn more about Cursor Agents

@cursor cursor bot dismissed stale reviews from salimtb and juanmigdr via 43b5233 December 15, 2025 12:38
@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Dec 15, 2025
Merged via the queue into main with commit 736915b Dec 15, 2025
287 of 288 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the cursor/notification-services-eslint-cleanup-901c branch December 15, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants