fix: notification services controller mobile fixes #4441
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this is because it breaks mobile tests (as the test environment is node, not web)
… integrated this is because mobile does not yet support push notifications
these are not usable with TS projects that use isolatedModules
Prithpal-Sooriya
added
the
team-notifications
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
we wanted to expose the nock mocks as it is nice for shared testing between clients, but this is an issue on mobile. Removing nock mocks for mobile. We can see if we can re-introduce this in the future.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Prithpal-Sooriya
force-pushed
the
fix/notification-services-fixes
branch
from
2eb7baf to
30646c8
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Prithpal-Sooriya
force-pushed
the
fix/notification-services-fixes
branch
from
30646c8 to
ab5de02
Compare
this helps sharing known notification chainID with our platforms
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| @@ -241,6 +241,9 @@ export default class NotificationServicesController extends BaseController< | |||
| NotificationServicesControllerState, | |||
| NotificationServicesControllerMessenger | |||
| > { | |||
| // Temporary boolean as push notifications are not yet enabled on mobile | |||
| #isPushIntegrated = true; | |||
There was a problem hiding this comment.
Temp for mobile. We will remove this in a later release
There was a problem hiding this comment.
Push Notification controllers isn't integrated and won't be on Mobile since MM app relies on Firebase SDK directly. Maybe we should remove the comment and use this flag as a feature that it is.
There was a problem hiding this comment.
OK fair, as this is 0.x.x packages, there most likely will be more changes coming down the pipeline. I shall update any documentation and comments in a future version.
| @@ -3,4 +3,3 @@ export * from './mock-notification-trigger'; | |||
| export * from './mock-notification-user-storage'; | |||
| export * from './mock-raw-notifications'; | |||
| export * from './mockResponses'; | |||
| export * from './mockServices'; | |||
There was a problem hiding this comment.
mockServices used nock, but was for some reason incompatible with mobile. Removed for now. And we can add back in a later release once we figure out multiple exports and tree shaking.
Comment on lines
+47
to
+62
| export const NOTIFICATION_CHAINS_ID = { | ||
| ETHEREUM: '1', | ||
| OPTIMISM: '10', | ||
| BSC: '56', | ||
| POLYGON: '137', | ||
| ARBITRUM: '42161', | ||
| AVALANCHE: '43114', | ||
| LINEA: '59144', | ||
| }; | ||
| } as const; | ||
|
|
||
| type ToPrimitiveKeys<TObj> = Compute<{ | ||
| [K in keyof TObj]: TObj[K] extends string ? string : TObj[K]; | ||
| }>; | ||
| export const NOTIFICATION_CHAINS: ToPrimitiveKeys< | ||
| typeof NOTIFICATION_CHAINS_ID | ||
| > = NOTIFICATION_CHAINS_ID; |
There was a problem hiding this comment.
This is some fancy types/constants so they can be reused for the /ui folder.
NOTIFICATION_CHAINS_ID is a strict object, whereas NOTIFICATION_CHAINS is the original loose object.
Comment on lines
+1
to
+23
| import { NOTIFICATION_CHAINS_ID } from '../constants/notification-schema'; | ||
|
|
||
| export const NOTIFICATION_NETWORK_CURRENCY_NAME = { | ||
| [NOTIFICATION_CHAINS_ID.ETHEREUM]: 'Ethereum', | ||
| [NOTIFICATION_CHAINS_ID.ARBITRUM]: 'Arbitrum', | ||
| [NOTIFICATION_CHAINS_ID.AVALANCHE]: 'Avalanche', | ||
| [NOTIFICATION_CHAINS_ID.BSC]: 'Binance', | ||
| [NOTIFICATION_CHAINS_ID.LINEA]: 'Linea', | ||
| [NOTIFICATION_CHAINS_ID.OPTIMISM]: 'Optimism', | ||
| [NOTIFICATION_CHAINS_ID.POLYGON]: 'Polygon', | ||
| } as const; | ||
|
|
||
| export const NOTIFICATION_NETWORK_CURRENCY_SYMBOL = { | ||
| [NOTIFICATION_CHAINS_ID.ETHEREUM]: 'ETH', | ||
| [NOTIFICATION_CHAINS_ID.ARBITRUM]: 'ETH', | ||
| [NOTIFICATION_CHAINS_ID.AVALANCHE]: 'AVAX', | ||
| [NOTIFICATION_CHAINS_ID.BSC]: 'BNB', | ||
| [NOTIFICATION_CHAINS_ID.LINEA]: 'ETH', | ||
| [NOTIFICATION_CHAINS_ID.OPTIMISM]: 'ETH', | ||
| [NOTIFICATION_CHAINS_ID.POLYGON]: 'MATIC', | ||
| }; | ||
|
|
||
| export { NOTIFICATION_CHAINS_ID } from '../constants/notification-schema'; |
There was a problem hiding this comment.
Mobile does not have consistent token name/exports to extension. To keep things in sync, we will use these shared constants in this package.
| @@ -69,6 +69,9 @@ export type NotificationServicesPushControllerMessenger = | |||
| AllowedEvents['type'] | |||
| >; | |||
|
|
|||
| export const defaultState: NotificationServicesPushControllerState = { | |||
There was a problem hiding this comment.
Mobile ask - we are now exporting default exports (this ensures that our redux initial state has "something").
| @@ -12,7 +12,7 @@ import type { Types } from '../../../NotificationServicesController'; | |||
| import { Processors } from '../../../NotificationServicesController'; | |||
| import type { PushNotificationEnv } from '../../types/firebase'; | |||
|
|
|||
| const sw = self as unknown as ServiceWorkerGlobalScope; | |||
| declare const self: ServiceWorkerGlobalScope; | |||
There was a problem hiding this comment.
self does not exist on mobile and breaks. So instead removing the sw variable and self is only used within functions instead of a global variable.
In the future (once we support multiple exports and tree shaking) we can tidy this up.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "extends": "../../tsconfig.packages.build.json", | |||
| "extends": "../../tsconfig.packages.json", | |||
There was a problem hiding this comment.
Misc, noticed we are not using the correct tsconfig for this file compared to other packages. No major issues though.
| @@ -1,6 +1,6 @@ | |||
| import type { Env, Platform } from '../env'; | |||
|
|
|||
| export const enum AuthType { | |||
| export enum AuthType { | |||
There was a problem hiding this comment.
Infura SDK ask. Projects that use isolatedModules are not compatible with const enums.
Jonathansoufer
approved these changes
Jun 27, 2024
| @@ -241,6 +241,9 @@ export default class NotificationServicesController extends BaseController< | |||
| NotificationServicesControllerState, | |||
| NotificationServicesControllerMessenger | |||
| > { | |||
| // Temporary boolean as push notifications are not yet enabled on mobile | |||
| #isPushIntegrated = true; | |||
There was a problem hiding this comment.
Push Notification controllers isn't integrated and won't be on Mobile since MM app relies on Firebase SDK directly. Maybe we should remove the comment and use this flag as a feature that it is.
Prithpal-Sooriya
added a commit
that referenced
this pull request
Jun 27, 2024
## Explanation This PR create a new release for - `@metamask/profile-sync-controller` to `^0.2.0` - `@metamask/notification-services-controller` to `^0.2.0` ## References Fixes mobile integration issues. ## Changelog ### `@metamask/profile-sync-controller` - **CHANGED**: Tidied up and small refactors to support mobile integration ([#4441](#4441)) ### `@metamask/notification-services-controller` - **CHANGED**: Tidied up and small refactors to support mobile integration ([#4441](#4441)) ## 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 highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Jongsun Suh <[email protected]>
Prithpal-Sooriya
added a commit
that referenced
this pull request
Jun 27, 2024
## Explanation This PR create a new release for. This is a re-opened release from a revert (apologies again!) #4471 - `@metamask/profile-sync-controller` to `^0.1.1` - `@metamask/notification-services-controller` to `^0.1.1` ## References Fixes mobile integration issues. ## Changelog ### `@metamask/profile-sync-controller` - **CHANGED**: Tidied up and small refactors to support mobile integration ([#4441](#4441)) ### `@metamask/notification-services-controller` - **CHANGED**: Tidied up and small refactors to support mobile integration ([#4441](#4441)) ## 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 highlighted breaking changes using the "BREAKING" category above as appropriate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
This PR contains a bunch of mobile fixes (when integrating this controller)
Removed self. This is because it breaks mobile tests (as the mobile test environment is node, not web)
References
N/A
Changelog
@metamask/package-a@metamask/package-bChecklist