[SDK-3916] Update publishing methods to accept a Message-shaped object
#1515
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
temporarily deployed
to
staging/pull/1515/features
github-actions
bot
temporarily deployed
to
staging/pull/1515/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1515/typedoc
owenpearson
requested changes
Nov 28, 2023
There was a problem hiding this comment.
Looks cool but I'm a bit unsure why we would even keep Types.Message here when it's now only referenced in a couple of places and is essentially just an alias to MessageLike (I realise that one's a class and one's an interface but I can't think of any situation where it would be useful to have both)? I think it might actually be better developer experience to dropclass Message and then rename MessageLike to Message - mainly on the basis that MessageLike might be a slightly non-obvious name for some, wdyt?
.
This makes us consistent with the IDL in the feature spec. (The only properties that are guaranteed to be populated on a message received from Ably are `id` and `timestamp`.)
The current type of `any` for an outgoing message is overly permissive and doesn’t help users understand the shape of the object they need to provide. So, we: 1. change the Message class to an interface, which represents a Message-shaped object; 2. make Message’s `id` and `timestamp` properties optional (since we’re going to also use this interface for outgoing messages, which don’t necessarily have these properties), and compensate for this loosening of the Message type by introducing an InboundMessage type to represent messages received from Ably; 3. update the signature publishing methods to accept a Message object. Note that we deviate from the feature spec in that, in the feature spec, the publishing methods accept a Message instance. There are a couple of reasons for this deviation: 1. Accepting a Message-shaped object instead of a Message instance is consistent with our usage of the library in all of our existing example code and our tests, and is, I think, how things tend to be done in JavaScript. 2. The types in the feature spec are wrong; as things stand there, a user needs to provide a Message to the publishing methods, but Message has non-optional `id` and `timestamp` properties even though a user is not expected to populate them. We haven’t yet figured out how to address this error in the feature spec (i.e. do we introduce an InboundMessage type like we have here, or do we add some comments and leave it for library authors to figure out?); I started a dicussion about it in [1], but we’ve decided that we’d like to proceed with this ably-js change (which, since it’s a breaking change, we want to get into v2) without waiting for it to be addressed in the feature spec. Resolves #1472. [1] ably/specification#156
lawrence-forooghian
force-pushed
the
1472-publish-accept-message
branch
from
410133b
to
19a8d47
Compare
|
@owenpearson makes sense; I've incorporated your suggestions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the optionality of some properties of the
Messageclass and then updates the publishing methods to accept aMessage-shaped object. See commit messages for more details.Resolves #1472.