Uh oh!
There was an error while loading. Please reload this page.
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 2 commits into
Conversation
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.
returnhttp2SessionHandler.invoke().catch((error)=>{thrownewFirebaseMessagingSessionError(error,undefined,undefined);}).then(()=>{constrequests=registrationTokensArray.map(async(registrationId)=>{letrequestPath=`/v1/projects/${projectId}/registrations/${registrationId}/topicSubscriptions`;if(isSubscribe){requestPath+=`?topic_name=${topicName}`;}else{requestPath+=`/${topicName}?allow_missing=true`;}returnthis.messagingRequestHandler.invokeHttp2RequestHandler(FCM_SEND_HOST,requestPath,httpMethod,isSubscribe ? {} : undefined,http2SessionHandler);});returnPromise.allSettled(requests);}).then((results)=>{if(results.length>0&&results.every((r)=>r.status==='rejected')){constfirstReason=(results[0]asPromiseRejectedResult).reason;if(firstReasoninstanceofRequestResponseError){throwcreateFirebaseError(firstReason);}else{throwfirstReason;}}constresponse: MessagingTopicManagementResponse={successCount: 0,failureCount: 0,errors: [],};results.forEach((result,index)=>{if(result.status==='fulfilled'){response.successCount+=1;}else{response.failureCount+=1;consterr=result.reason;consterrorCode=err.response?.isJson() ? getErrorCode(err.response.data) : null;consterrorMessage=err.response?.isJson() ? err.response.data?.error?.message : err.message;constnewError=FirebaseMessagingError.fromTopicManagementServerError(errorCode||'UNKNOWN',errorMessage,err.response?.isJson() ? err.response.data : undefined);response.errors.push({
index,error: newError,});}});returnresponse;}).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.
…s via HTTP/2 multiplexing
9cadaf5 to
83f1db7Compare
Migrate topic subscription APIs to new REST endpoints via HTTP/2 multiplexing
DO_NOT_MERGE