-
Notifications
You must be signed in to change notification settings - Fork 45
Feature/pub 1676 message annotations #2642
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
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Couple comments & suggestions but generally looks good 👍
will leave it to the docs team to approve.
8d7eb0c
to
ed9ed44
Compare
@m-hulbert for some reason the left nav menu gets squished when the content is too wide, despite the fact that it seems to wrap okay |
In 9582fcf I have added the error codes used by message annotations, which have been updated in ably/ably-common#307 And aligned in realtime in: https://github.com/ably/realtime/pull/7514 We need to decide on the name used for the channel rule in the website and can align the error codes, error messages used in realtime and the docs here accordingly. |
In 66a92ed I have renamed the "Mutable messages" channel rule to "Advanced message features" per ADR-144 (pending decision) and included the appropriate caveats for features that are not yet supported on mutable messages enabled channels. |
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.
Thanks Mike, this is comprehensive in terms of content - I just think shuffling the content around might help in understanding it a little quicker.
I know I've made various suggestions throughout, but in hindsight I wonder if the entire page should speak solely in terms of message summaries considering that is what we recommend. Then we have an independent section at the end that explains how/why you can subscribe to individual events. Otherwise it becomes quite complex to explain both instances everywhere. This would make some of my comments obsolete, but let me know what you think.
Also, the content squish is a regression in the MDX implementation which I've logged with the web team. It's related to codeblock scrolling/wrapping of long lines where we've added line numbering.
<Aside data-type="info"> | ||
When using message annotations, normal messages delivered to the [`subscribe()`](/docs/pub-sub#subscribe) listener will have an `action` of `message.create`. | ||
</Aside> |
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.
I wonder if this should be part of the intro section, but more importantly clearly state that annotation (summaries) use the same listener as regular messages on the channel. WDYT?
<Aside data-type='note'> | ||
Individual annotation events are useful for activity feeds or detailed logging, but for maintaining UI state, summary events are generally more reliable and efficient. | ||
</Aside> |
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.
I'd pull this up into the intro of the section and out of the aside.
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.
I'm rewriting this para. "More reliable" is not right (receiving raw annotations is perfectly reliable)
Agree with Simon's comment, otherwise this LGTM @m-hulbert, reads nicely, thanks :) |
0125c44
to
612c0c0
Compare
Made the update to the modes. I also updated the channel rule with the latest terminology, and rebased to fix the issue with the page widths, so it should be readable in the review app now. |
612c0c0
to
298a0ef
Compare
I've rebased this on main (somewhat painfully) and updated it for protocol v4. (Our docs will just unconditionally use protocol v4 message docs for annotations and updates, since by the time we release those features experimentally all 3 sdks which had support for them will be updated to v4). I didn't keep the previous commit structure; there were a lot of fixup commits that didn't cleanly apply to the commit they were fixing up given other ones inbetween. I squashed it down to one commit nominally written by @mschristensen , and added my changes as a new commit after that. |
4314f2b
to
769c246
Compare
And remove references to `version` for now until we add the update/delete docs. Our docs will just unconditionally use protocol v4 message docs for annotations and updates, since by the time we release those features experimentally all 3 sdks which had support for them will be updated to v4.
Removed 'Publishing individual annotation events' section separate from 'annotation publish', I think that's confusing. Just mentioned that you can specify a data payload at the bottom of the publish section.
855fb67
to
8ddcc34
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 - thanks for taking the hit on the rebase @SimonWoolf
Description
Adds docs for the message annotations feature to the Messages section of the Pub/Sub product docs.
Includes:
type
specifiers the different summarization methods and their semanticsNot ready to merge yet until we are ready to make the release but opening as draft now for early review.
Checklist