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 HTTP2 support for sendEach() and sendEachForMulticast() #2550

Merged
merged 11 commits into from
Jul 23, 2024

Conversation

jonathanedey
Copy link
Contributor

@jonathanedey jonathanedey commented May 9, 2024

Added:

  • HTTP/2 support for sendEach() and sendEachForMulitcast()
  • New deprecated method enableLegacyTransport()

When sending messages using sendEach() or sendEachForMulitcast() a HTTP/2 connection is now used by default.

In order to use the legacy HTTP/1.1 versions of these methods, the enableLegacyTransport() method must be used. This method is already marked as deprecated and will be removed once the HTTP/2 transport is considered fully stable.

Related: #2488

@jonathanedey jonathanedey added the release:stage Stage a release candidate label May 9, 2024
@jonathanedey jonathanedey changed the title feat: Add HTTP2 support for sendEach() and sendEachForMulticast feat: Add HTTP2 support for sendEach() and sendEachForMulticast() May 10, 2024
@jonathanedey jonathanedey marked this pull request as ready for review May 10, 2024 19:28
test/unit/utils/api-request.spec.ts Dismissed Show dismissed Hide dismissed
test/unit/utils/api-request.spec.ts Dismissed Show dismissed Hide dismissed
test/unit/utils/api-request.spec.ts Dismissed Show dismissed Hide dismissed
test/unit/utils/api-request.spec.ts Dismissed Show dismissed Hide dismissed
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.

Thanks, Jonathan!
The first pass (to quickly unblock you) LGTM. I will take another look at the tests.
Let's get an internal API proposal going soon

@lahirumaramba lahirumaramba changed the title feat: Add HTTP2 support for sendEach() and sendEachForMulticast() feat(fcm): Add HTTP2 support for sendEach() and sendEachForMulticast() May 30, 2024
@lahirumaramba
Copy link
Member

Thanks Jonathan! Let's make the changes we discussed in the API proposal and do another pass. We should be good to go then!

* ```
*
* @deprecated This is to be removed once the HTTP/2 transport is universally safe.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can we get a TW review on this?

* messaging.sendEach(messages);
* ```
*
* @deprecated This is to be removed once the HTTP/2 transport is universally safe.
Copy link
Member

Choose a reason for hiding this comment

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

This is to be removed once the HTTP/2 transport implementation is universally safe. ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm not crazy about that wording either. Can we give a more concrete criteria than safety?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Safe here was to imply when the HTTP/2 implementation was stable enough where it was on par with the HTTP/1.1 implementation and no longer needed an emergency back up in case some functionality was completely covered.

Went with the following but open to any suggestions here:
This is to be removed once the HTTP/2 transport implementation reaches the same stability as the legacy HTTP/1.1 implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

That works for me!

If it's easily done, I'd tone down to "This will be removed when..."

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! But I do have comments :)

Can you use your eagle eye and do a scrub for literals that lack backticks Lahiru? Thanks!

src/messaging/messaging.ts Outdated Show resolved Hide resolved
* messaging.sendEach(messages);
* ```
*
* @deprecated This is to be removed once the HTTP/2 transport is universally safe.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm not crazy about that wording either. Can we give a more concrete criteria than safety?

src/utils/api-request.ts Outdated Show resolved Hide resolved
src/utils/api-request.ts Show resolved Hide resolved
src/utils/api-request.ts Outdated Show resolved Hide resolved
src/utils/api-request.ts Outdated Show resolved Hide resolved
src/utils/api-request.ts Outdated Show resolved Hide resolved
src/utils/api-request.ts Outdated Show resolved Hide resolved
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! Thanks for fixing the literals :)

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