Realtime message send + push message send on channel#310
Realtime message send + push message send on channel#310umair-ably wants to merge 2 commits intomainfrom
Conversation
…commands targeting a channel
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR adds a Changes
Review Notes
|
There was a problem hiding this comment.
Review Summary
The feature is well-scoped: a --message flag for push publish that includes realtime message data alongside the push extras when publishing via --channel. The validation logic is correct (errors if --message used without --channel, warns if a direct recipient overrides --channel). Two issues worth addressing:
1. messageData: true in JSON output omits actual content
File: src/commands/push/publish.ts, lines 293–303
...(flags.message ? { messageData: true } : {}),The JSON result confirms that a message was included (messageData: true) but drops the actual content that was sent. A programmatic consumer using --json for auditing or pipeline verification can't tell what data was published — just that something was.
The fix is straightforward: emit the actual parsed value instead of the boolean sentinel:
...(flags.message ? { messageData: publishMessage.data } : {}),publishMessage.data is already set at this point (lines 283–288), so this costs nothing and makes the JSON output self-describing.
2. Non-standard test describe block
File: test/unit/commands/push/publish.test.ts, line 201
describe("--message flag (realtime message data)", () => {This is a sibling of "functionality" and "error handling" but doesn't match any of the 5 required standard block names. The new test cases belong in the existing describe("functionality" block (happy-path tests) and describe("error handling" block (the "fail without --channel" case). The test coverage itself is solid — it's just the placement that needs adjusting.
Both changes are small. The JSON output issue is the more meaningful one since it affects consumers relying on --json for automation.
| --fcm=<value> FCM-specific override as JSON | ||
| --icon=<value> Notification icon | ||
| --json Output in JSON format | ||
| --message=<value> Realtime message data to include alongside the push notification (only applies when |
There was a problem hiding this comment.
I'm not against this approach, but tbh it's a bit confusing. --channel command is a shortcut and should work as such. I think the proper way would be to put push --body as realtime message. WDYT?
There was a problem hiding this comment.
I don't think that works well for a few reasons...
- --channel isn't a shortcut... it's config option, same as client-id or device-id
- The push body can be different from the realtime message e.g. a user may wish to send some long form data for the app to consume, but wishes to show a much shorter push body e.g. push body = "your friend just reached a high score" and the realtime message could be "{message: "Jamie just beat your high score", score: 77, yourhighScore: 55}" or whatever else
There was a problem hiding this comment.
there was the option of using a positional arg so it would be
ably push publish --channel="test" --title="new notification" --body="notification body" "this is the realtime message"
but i think that is more confusing e.g. a user might mistake the positional arg for the notification body (and then there isn't an argument description like we have now iirc)
There was a problem hiding this comment.
Ok, what about fallback to body if message omitted?
There was a problem hiding this comment.
I think no realtime message but a notification to an entire channel as recipients is a valid use case?
There was a problem hiding this comment.
I'm not sure I understand your response, I mean if --message is omitted, then --body is being put to the realtime data field. Use case - when those two are the same, why have both?
There was a problem hiding this comment.
my point is that a user could want to publish a push notification to a realtime channel but have no realtime message associated with it.
If we send the --body as the realtime message, then that implies a user always has to send something to the realtime channel when doing push, which isn't true?
There was a problem hiding this comment.
If it's true, then this behavior should be supported server-side (or within SDK), because right now you've got an empty message fired. Look at this screenshot.
Allows sending a realtime message and a push message when using push commands targeting a channel.
Opted for a flag instead of a positional arg so it's clearer what the realtime message data is and what the push object is. There are also checks to guard against using the flag where it is incompatible e.g. using the message flag with a client id push.
Fixes #256