-
Notifications
You must be signed in to change notification settings - Fork 56
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
[REA-1702] feat: web push #1775
Conversation
owenpearson
commented
May 22, 2024
- adds a plugin which enables clients to be activated for receipt of push notifications.
- adds support for w3c push transport to web clients.
- adds push channel transport for testing push activation on all platforms.
df841d6
to
9175832
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! very consistent to what we have in java and cocoa, added couple of small comments
9175832
to
05af39c
Compare
05af39c
to
6970b80
Compare
6970b80
to
934a818
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a wall of comments but mostly they are structural suggestions.
Couple of things that I couldn't attach to code lines:
- Should we add new Push plugin to
scripts/moduleReport.ts
pluginNames
list? I imagine we would need to updategetBundleInfo
in there, as currently it always imports from'./build/modular/index.mjs'
. - Per RSH7 both
RestChannel
andRealtimeChannel
should havepush
object. Why did we addpush
only toRestChannel
here?
P.S. sorry for the amount of comments 😬 I wonder if we could've done it in separate PRs for web push integration branch.
934a818
to
7a2eb66
Compare
7a2eb66
to
88389e4
Compare
88389e4
to
c4eaecc
Compare
c481002
to
20778ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM