feat(fcm): Enable fid and deprecate token for Send API#3145
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Firebase Installation IDs (FIDs) in the messaging service by adding the FidMessage interface and updating the Message union type. The MulticastMessage interface now supports targeting via fids, and the tokens field has been deprecated and made optional. Feedback includes suggestions to add documentation for the new FidMessage interface, refactor the message construction logic using object destructuring for better maintainability, and improve the grammar of validation error messages. Additionally, the reviewer noted that making tokens optional is a breaking change for TypeScript consumers that should be highlighted in release notes.
| export interface FidMessage extends BaseMessage { | ||
| fid: string; | ||
| } |
There was a problem hiding this comment.
To address your question in the PR description, it is highly recommended to add docstrings for the new FidMessage interface and its fields. This ensures consistency with other public interfaces and improves the developer experience for users of the SDK.
/**
* Interface representing a message targeted at a Firebase Installation ID (FID).
*/
export interface FidMessage extends BaseMessage {
/**
* The Firebase Installation ID (FID) to which the message should be sent.
*/
fid: string;
}| export interface MulticastMessage extends BaseMessage { | ||
| tokens: string[]; | ||
| /** | ||
| * A list of Firebase Installation IDs (FIDs) to target. | ||
| */ | ||
| fids?: string[]; | ||
|
|
||
| /** | ||
| * A list of registration tokens to target. | ||
| * | ||
| * @deprecated Use {@link MulticastMessage.fids} instead. | ||
| */ | ||
| tokens?: string[]; | ||
| } |
There was a problem hiding this comment.
Changing tokens from a required field (tokens: string[]) to an optional one (tokens?: string[]) in the MulticastMessage interface is a breaking change for TypeScript consumers who might be relying on the property's presence (e.g., accessing .length without a null check). While necessary for the migration to fids, this should be clearly highlighted in the release notes.
| throw new FirebaseMessagingError( | ||
| MessagingClientErrorCode.INVALID_ARGUMENT, | ||
| `tokens list must not contain more than ${FCM_MAX_BATCH_SIZE} items`); | ||
| `tokens and fids list must not contain more than ${FCM_MAX_BATCH_SIZE} items in total`); |
There was a problem hiding this comment.
The error message grammar can be improved for better clarity.
| `tokens and fids list must not contain more than ${FCM_MAX_BATCH_SIZE} items in total`); | |
| `The total number of tokens and fids must not exceed ${FCM_MAX_BATCH_SIZE}.`); |
| const messages: Message[] = []; | ||
| tokens.forEach((token) => { | ||
| messages.push({ | ||
| token, | ||
| android: copy.android, | ||
| apns: copy.apns, | ||
| data: copy.data, | ||
| notification: copy.notification, | ||
| webpush: copy.webpush, | ||
| fcmOptions: copy.fcmOptions, | ||
| }; | ||
| }); | ||
| }); | ||
| fids.forEach((fid) => { | ||
| messages.push({ | ||
| fid, | ||
| android: copy.android, | ||
| apns: copy.apns, | ||
| data: copy.data, | ||
| notification: copy.notification, | ||
| webpush: copy.webpush, | ||
| fcmOptions: copy.fcmOptions, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The logic for constructing the messages array can be refactored to be more concise and future-proof by using object destructuring and the spread operator. This avoids manually listing every property of BaseMessage and automatically handles any future additions to the base type.
const { tokens: _, fids: __, ...baseMessage } = copy;
const messages: Message[] = [
...tokens.map((token) => ({ ...baseMessage, token } as Message)),
...fids.map((fid) => ({ ...baseMessage, fid } as Message)),
];1 participant
This change deprecates
TokenMessageinterface and thetokensfield inMulticastMessageinterface as message targets, transitioning support toFidMessageinterface andfidsinMulticastMessageas the primary targets instead.The multicast helper function
sendEachForMulticastis updated to support both target types concurrently up to a combined total of 500.Question: There were no docstrings for
TopicMessageandConditionMessageinterfaces previously. Should I add docstring for the newFidMessageinterface or I just stay consistent with the old interfaces?