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

RFC: Distinguish the different meanings of a "message" #156

Draft
wants to merge 1 commit into
base: merge-main-into-3.0.0
Choose a base branch
from

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Jul 18, 2023

The context for this suggested change is ably/ably-js#1398. There, I pointed out that the specification’s current signatures for publish (specifically, the overloads that accept a Message or an array thereof) do not seem to match how we’re expecting these methods to be used in real life. This is because Message’s id and timestamp properties are specified as non-optional, meaning that a user calling publish would need to populate these properties. In reality, we do not expect a user to populate these properties — they are usually generated by the Ably service.

The easiest way to solve this would be to be to make these properties optional. However, doing so would unnecessarily remove some useful type information for recipients of messages — the type system would no longer communicate that these properties are guaranteed to be populated in a message emitted by the library.

In this PR, I’m proposing that we distinguish between three separate concepts of a "message", which I think are perhaps currently being incorrectly conflated:

  1. The data type that a user of the library passes to the publish methods

  2. The data type that the library emits from methods that expose messages published on a channel

  3. The data type that describes a serialized message that is transmitted over the wire

I’ve named the first one OutgoingMessage, the second one IncomingMessage, and I believe that the third belongs in the documentation for the Ably protocol, not the library specification.

OutgoingMessage and IncomingMessage differ from the existing Message type in the following ways:

  • OutgoingMessage’s id and timestamp properties are now optional
  • OutgoingMessage does not have fromEncoded* methods
     
  • IncomingMessage’s connnectionId property is now non-optional (i.e. we are now able to provide stronger type information for this property) — I need to double-check whether this property is actually guaranteed to be populated by the library; my reading of TM2c and RTL6f suggested that it is, but I’m not sure if TM2c’s "the @connectionId@ of the encapsulating @ProtocolMessage@" is guaranteed to be populated.
  • IncomingMessage does not have constructors

I have not yet introduced spec points for these two new types — I will do so if there is a consensus to move forwards with this approach. For now, see the changes to the IDL.

Other thoughts:

  • I think that, similarly to the Message wire type, the ProtocolMessage type should also only be described by the protocol documentation, and not the feature spec.

  • If we do choose to start leaning more heavily on the protocol documentation, then we’ll need to bring it up to date — it looks like it hasn’t been touched in quite some time and still mentions connectionSerial, for example.

  • I’ve kept the exact same list of properties in IncomingMessage and OutgoingMessage, since my reading of RSL1j is that a user publishing a message should be able to populate any of the properties of the message that eventually gets sent over the wire. But if that’s not the case, then we may be able to remove some properties from OutgoingMessage.

@github-actions github-actions bot temporarily deployed to staging/pull/156 July 18, 2023 18:07 Inactive
@lawrence-forooghian lawrence-forooghian changed the title Split message in two Distinguish the different concepts of a "message" Jul 18, 2023
@lawrence-forooghian lawrence-forooghian changed the title Distinguish the different concepts of a "message" Distinguish the different meanings of a "message" Jul 18, 2023
@lawrence-forooghian lawrence-forooghian changed the title Distinguish the different meanings of a "message" RFC: Distinguish the different meanings of a "message" Jul 18, 2023
@github-actions github-actions bot temporarily deployed to staging/pull/156 July 18, 2023 19:25 Inactive
@lawrence-forooghian
Copy link
Collaborator Author

@lmars @SimonWoolf — added you as reviewers because I thought you might have opinions, but feel free to ignore.

@SimonWoolf
Copy link
Member

SimonWoolf commented Jul 18, 2023

my thoughts:

  • Tightning up the user-visible typings for messages on publish vs subscribe seems like an improvement, sure. But I'm not convinced making them two entirely different classes (in the spec or user-visibly) is the way forward, that just seems more confusing for the user.

    • Besides, many -- actually most -- sdks are in languages that don't represent nullability in their type systems, and having the spec require that those languages implement two different classes for incoming and outgoing that'd be ~identical would be unnecessary
    • How about we just tweak the spec IDL to say id: String // TM2a, optional for Message objects provided by the user, which sdks can interpret however they want? and then eg in ably-js expose a type that's still a Message, just tweaked to give you non-optional fields, ie type IncomingMessage = Message & Required<Pick<Message, 'id' | 'timestamp'>>
  • connnectionId is not guaranteed to be populated (either in the ProtocolMessage or the Message),eg because the message could be from a REST publish which has no connectionId because it isn't a connection

  • I'm kindof tempted to say that the lesson we could lean from the fact that the protocol page has barely been touched or read for years was that.. the attempt at having that split between publically-visible behaviour and the wire protocol was maybe a bad idea? Like -- a clean split is near-impossible, describing publically-visible behaviour necessarily needs to specify lots of internal behaviour too -- so you end up with the features spec that over time specifiec more and more internal bits, so fewer people ever look at /protocol, so it doesn't get updated and more things just go in the features spec, self-reinforcing cycle. Maybe we should just give up and pull all relevant specifications into the features spec (and rename it just 'the sdk spec'), so at least it'll be visible, read and updated as part of spec changes?

@lawrence-forooghian lawrence-forooghian changed the base branch from integration/3.0.0 to merge-main-into-3.0.0 July 19, 2023 17:17
@github-actions github-actions bot temporarily deployed to staging/pull/156 July 19, 2023 17:18 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/156 July 19, 2023 17:24 Inactive
@lawrence-forooghian
Copy link
Collaborator Author

lawrence-forooghian commented Jul 19, 2023

I'm not convinced making them two entirely different classes (in the spec or user-visibly) is the way forward

Is your concern literally about the fact that there would be two classes, or the fact that there would be two classes whose definitions are almost identical? If the latter, how would you feel about, for example, having incoming and outgoing message classes which subclass some common class?

Besides, many -- actually most -- sdks are in languages that don't represent nullability in their type systems, and having the spec require that those languages implement two different classes for incoming and outgoing that'd be ~identical would be unnecessary

These languages could choose not to implement two different classes, and just have one?

How about we just tweak the spec IDL to say id: String // TM2a, optional for Message objects provided by the user, which sdks can interpret however they want? and then eg in ably-js expose a type that's still a Message, just tweaked to give you non-optional fields, ie type IncomingMessage = Message & Required<Pick<Message, 'id' | 'timestamp'>>

Given that the IDL does represent nullability, would this not need to be id: String? // TM2a, guaranteed to be populated for messages received from Ably?

connnectionId is not guaranteed to be populated (either in the ProtocolMessage or the Message),eg because the message could be from a REST publish which has no connectionId because it isn't a connection

Thanks, I've updated my changes.

a clean split is near-impossible, describing publically-visible behaviour necessarily needs to specify lots of internal behaviour too

I'm not sure what you mean here. Is it that in order to specify how the client library should be implemented (i.e. what's currently described by the feature spec) one must necessarily describe how the Realtime service behaves and some ways in which a client must behave in order to correctly interact with it? That's true, for sure. But what about the other way round — do you believe that it's possible to describe the protocol in a manner that is more concise and understandable than describing exactly how the client library must be implemented? And if so, do you believe that there is value in that — perhaps, for example, from a didactic point of view, to help people understand the motivation behind the implementation requirements described by the feature spec? The feature spec definitely has a "handed down from the gods" vibe at the moment, and I wonder whether empowering developers to understand why it is the way it is might be a good thing.

The context for this suggested change is ably-js#1398. There, I pointed out
that the specification’s current signatures for `publish` (specifically, the
overloads that accept a `Message` or an array thereof) do not seem to match how
we’re expecting these methods to be used in real life. This is because
`Message`’s `id` and `timestamp` properties are specified as non-optional,
meaning that a user calling `publish` would need to populate these properties.
In reality, we do not expect a user to populate these properties — they are
usually generated by the Ably service.

The easiest way to solve this would be to be to make these properties optional.
However, doing so would unnecessarily remove some useful type information for
_recipients_ of messages — the type system would no longer communicate that
these properties are guaranteed to be populated in a message emitted by the
library.

In this PR, I’m proposing that we distinguish between three separate
concepts of a "message", which I think are perhaps currently being incorrectly
conflated:

1. The data type that a user of the library passes to the `publish` methods

2. The data type that the library emits from methods that expose messages
   published on a channel

3. The data type that describes a serialized message that is transmitted over
   the wire

I’ve named the first one OutgoingMessage, the second one IncomingMessage, and I
believe that the third belongs in the documentation for the Ably protocol, not
the library specification.

OutgoingMessage and IncomingMessage differ from the existing `Message` type in
the following ways:

- OutgoingMessage’s `id` and `timestamp` properties are now optional
- OutgoingMessage does not have `fromEncoded*` methods

- IncomingMessage does not have constructors

I have not yet introduced spec points for these two new types — I will do so if
there is a consensus to move forwards with this approach. For now, see the
changes to the IDL.

Other thoughts:

- I think that, similarly to the Message wire type, the ProtocolMessage type
  should also only be described by the protocol documentation, and not the
  feature spec.

- If we do choose to start leaning more heavily on the protocol documentation,
  then we’ll need to bring it up to date — it looks like it hasn’t been touched
  in quite some time and still mentions `connectionSerial`, for example.

- I’ve kept the exact same list of properties in IncomingMessage and
  OutgoingMessage, since my reading of RSL1j is that a user publishing a
  message should be able to populate any of the properties of the message that
  eventually gets sent over the wire. But if that’s not the case, then we may be
  able to remove some properties from `OutgoingMessage`.
@lawrence-forooghian
Copy link
Collaborator Author

We're addressing this in ably-js in line with your suggestion, @SimonWoolf: ably/ably-js#1515.

lawrence-forooghian added a commit to ably/ably-js that referenced this pull request Nov 29, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants