Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(fcm): Add sendEach and sendEachForMulticast for FCM batch send #2138

Merged
merged 5 commits into from
Apr 12, 2023

Conversation

Doris-Ge
Copy link
Contributor

@Doris-Ge Doris-Ge commented Apr 5, 2023

  • Deprecate sendAll() and sendMulticast() APIs.
  • Add new sendEach() and sendEachForMulticast() APIs.

RELEASE NOTE: sendAll() and sendMulticast() APIs are now deprecated. Please use sendEach() and sendEachForMulticast() APIs instead.

1. Deprecate sendAll and sendMulticast
2. Add dummy implementation for sendEach and sendEachForMulticast to avoid errors reported by api-extractor
`sendEach` vs `sendAll`
1. `sendEach` sends one HTTP request to V1 Send endpoint for each
    message in the array.
   `sendAll` sends only one HTTP request to V1 Batch Send endpoint
    to send all messages in the array.
2. `sendEach` calls `Promise.allSettled` to wait for all
   `httpClient.send` calls to complete and construct a `BatchResponse`.
    An `httpClient.send` call to V1 Send endpoint either completes
    with a success or throws an error. So if an error is thrown out,
    the error will be caught in `sendEach` and turned into a
    `SendResponse` with an error.
    Therefore, unlike `sendAll`, `sendEach` does not always throw
    an error for a total failure. It can also return a `BatchResponse`
    with only errors in it.

`sendEachForMulticast` calls `sendEach` under the hood.
@lahirumaramba lahirumaramba changed the title sendEach and sendEachForMulticast for FCM batch send feat(fcm): Add sendEach and sendEachForMulticast for FCM batch send Apr 5, 2023
@lahirumaramba lahirumaramba added release-note release:stage Stage a release candidate labels Apr 5, 2023
@lahirumaramba
Copy link
Member

Thank you @Doris-Ge . Let's get the docs reviewed by @egilmorez before we merge this.

Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

LG with one optional nit, thanks!

*
* The responses list obtained from the return value corresponds to the order of `messages`.
* An error from this method or a `BatchResponse` with all failures indicates a total failure
* -- i.e. none of the messages in the list could be sent. Partial failures or no failures
Copy link
Contributor

@egilmorez egilmorez Apr 11, 2023

Choose a reason for hiding this comment

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

We avoid i. e., so for this I'd suggest just "...indicates a total failure, meaning that none of the messages in the list could be sent.

src/messaging/messaging.ts Outdated Show resolved Hide resolved
@Doris-Ge
Copy link
Contributor Author

Thanks Eric for reviewing the PR! @lahirumaramba I tried to push the fix to the fcm-batch-send branch but it got rejected because "Protected branch update failed for refs/heads/fcm-batch-send". Should I create another PR from dorisge/fcm-batch-send for the fix and merge it to fcm-batch-send?

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:stage Stage a release candidate release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants