WIP feat(fcm): Migrate topic subscription APIs to new REST endpoints#3136
WIP feat(fcm): Migrate topic subscription APIs to new REST endpoints#3136lahirumaramba wants to merge 1 commit into
Conversation
…s via HTTP/2 multiplexing
There was a problem hiding this comment.
Code Review
This pull request refactors topic management to use the FCM v1 HTTP/2 API, replacing the legacy batch IID endpoint with individual requests per registration token. Feedback focuses on refactoring the asynchronous logic to avoid the new Promise constructor anti-pattern, ensuring the HTTP/2 session is established before requests are sent, and using defined constants instead of hardcoded host strings for better maintainability.
| let settledPromise: Promise<PromiseSettledResult<any>[]>; | ||
| return new Promise<MessagingTopicManagementResponse>((resolve, reject) => { | ||
| http2SessionHandler.invoke().catch((error) => { | ||
| reject(new FirebaseMessagingSessionError(error, undefined, undefined)); | ||
| }); | ||
|
|
||
| const requests = registrationTokensArray.map(async (registrationId) => { | ||
| let requestPath = `/v1/projects/${projectId}/registrations/${registrationId}/topicSubscriptions`; | ||
| if (isSubscribe) { | ||
| requestPath += `?topic_name=${topicName}`; | ||
| } else { | ||
| requestPath += `/${topicName}?allow_missing=true`; | ||
| } | ||
| return this.messagingRequestHandler.invokeHttp2RequestHandler( | ||
| 'fcm.googleapis.com', | ||
| requestPath, | ||
| httpMethod, | ||
| isSubscribe ? {} : undefined, | ||
| http2SessionHandler | ||
| ); | ||
| }); | ||
|
|
||
| settledPromise = Promise.allSettled(requests); | ||
| settledPromise.then((results) => { | ||
| if (results.length > 0 && results.every((r) => r.status === 'rejected')) { | ||
| const firstReason = (results[0] as PromiseRejectedResult).reason; | ||
| if (firstReason instanceof RequestResponseError) { | ||
| reject(createFirebaseError(firstReason)); | ||
| } else { | ||
| reject(firstReason); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const response: MessagingTopicManagementResponse = { | ||
| successCount: 0, | ||
| failureCount: 0, | ||
| errors: [], | ||
| }; | ||
|
|
||
| results.forEach((result, index) => { | ||
| if (result.status === 'fulfilled') { | ||
| response.successCount += 1; | ||
| } else { | ||
| response.failureCount += 1; | ||
| const err = result.reason; | ||
| const errorCode = err.response?.isJson() ? getErrorCode(err.response.data) : null; | ||
| const errorMessage = err.response?.isJson() ? err.response.data?.error?.message : err.message; | ||
| const newError = FirebaseMessagingError.fromTopicManagementServerError( | ||
| errorCode || 'UNKNOWN', | ||
| errorMessage, | ||
| err.response?.isJson() ? err.response.data : undefined | ||
| ); | ||
| response.errors.push({ | ||
| index, | ||
| error: newError, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| resolve(response); | ||
| }); | ||
| }).finally(() => { | ||
| http2SessionHandler.close(); | ||
| }); |
There was a problem hiding this comment.
The current implementation uses the new Promise constructor anti-pattern to wrap existing promise-returning logic. Additionally, it initiates all HTTP requests immediately, even if the HTTP/2 session invocation fails, which is inefficient and can lead to unhandled rejections.
Refactoring this into a clean promise chain ensures that requests are only started after the session is successfully established and simplifies error handling.
return http2SessionHandler.invoke()
.catch((error) => {
throw new FirebaseMessagingSessionError(error, undefined, undefined);
})
.then(() => {
const requests = registrationTokensArray.map(async (registrationId) => {
let requestPath = `/v1/projects/${projectId}/registrations/${registrationId}/topicSubscriptions`;
if (isSubscribe) {
requestPath += `?topic_name=${topicName}`;
} else {
requestPath += `/${topicName}?allow_missing=true`;
}
return this.messagingRequestHandler.invokeHttp2RequestHandler(
FCM_SEND_HOST,
requestPath,
httpMethod,
isSubscribe ? {} : undefined,
http2SessionHandler
);
});
return Promise.allSettled(requests);
})
.then((results) => {
if (results.length > 0 && results.every((r) => r.status === 'rejected')) {
const firstReason = (results[0] as PromiseRejectedResult).reason;
if (firstReason instanceof RequestResponseError) {
throw createFirebaseError(firstReason);
} else {
throw firstReason;
}
}
const response: MessagingTopicManagementResponse = {
successCount: 0,
failureCount: 0,
errors: [],
};
results.forEach((result, index) => {
if (result.status === 'fulfilled') {
response.successCount += 1;
} else {
response.failureCount += 1;
const err = result.reason;
const errorCode = err.response?.isJson() ? getErrorCode(err.response.data) : null;
const errorMessage = err.response?.isJson() ? err.response.data?.error?.message : err.message;
const newError = FirebaseMessagingError.fromTopicManagementServerError(
errorCode || 'UNKNOWN',
errorMessage,
err.response?.isJson() ? err.response.data : undefined
);
response.errors.push({
index,
error: newError,
});
}
});
return response;
})
.finally(() => {
http2SessionHandler.close();
});| const isSubscribe = methodName === 'subscribeToTopic'; | ||
| const httpMethod = isSubscribe ? 'POST' : 'DELETE'; | ||
|
|
||
| const http2SessionHandler = new Http2SessionHandler('https://fcm.googleapis.com'); |
There was a problem hiding this comment.
Migrate topic subscription APIs to new REST endpoints via HTTP/2 multiplexing
DO_NOT_MERGE