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

Update publish methods to accept a Message-like type (including specification changes) #1398

Open
lawrence-forooghian opened this issue Jul 18, 2023 · 0 comments
Labels
breaking Backwards incompatible changes made to the public API.

Comments

@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Jul 18, 2023

Note (2023-10-25) — see #1472, which covers this work without the specification changes.

Our type signatures for the implementations of the specification’s publish(Message) and publish([Message]) methods only require that their arguments be any and any[], respectively.

I’m not sure why this is the case. It seems very permissive, and whilst I haven’t looked into exactly what types the implementations are able to handle, I’m sure it is not in fact any.

I think that we should update these signatures to be more in line with those of the specification. The question then becomes what type we should use. We could update it to use the Message class, but in ably-js’s type declarations, the only way for a user to create a Message object is by using the fromEncoded or fromEncodedArray static methods. I don't think that we'd want the user to need to use those methods in order to be able to construct an argument for publish. Message is meant to have, per TM4, public constructors, but it doesn’t in ably-js.

We could choose to expose the Message constructor, but it might be more JS-like to instead accept a Message-shaped object — that is, to create a MessageLike interface.

The spec for publish doesn’t seem right

The specification shows Message’s id and timestamp properties as non-optional, but these are usually meant to be populated by the Ably service, and almost never by the user. So it doesn’t seem right to me that the user is meant to pass a Message instance to publish. I think we need to figure this out before choosing the ably-js type. See WIP PR ably/specification#156 for one possible approach.

Related issues

The same problem will apply when we come to implement the batch publish API in #989 — what type are we meant to use for BatchPublishSpec.messages?

┆Issue is synchronized with this Jira Task by Unito

@lawrence-forooghian lawrence-forooghian added the breaking Backwards incompatible changes made to the public API. label Jul 18, 2023
@lawrence-forooghian lawrence-forooghian changed the title Update publish methods to accept a Message-like type Update publish methods to accept a Message-like type (including specification changes) Oct 25, 2023
@ably ably deleted a comment from sync-by-unito bot Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Backwards incompatible changes made to the public API.
Development

No branches or pull requests

1 participant