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.

Sign up for GitHub

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

Jump to bottom

Update publish methods to accept a Message-like type (without specification changes) #1472

Closed
lawrence-forooghian opened this issue Oct 25, 2023 · 2 comments
Closed

Update publish methods to accept a Message-like type (without specification changes) #1472

lawrence-forooghian opened this issue Oct 25, 2023 · 2 comments
Assignees
@lawrence-forooghian

Comments

@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Oct 25, 2023 •
edited
Loading

We decided today that we would like to implement the change described in #1398 for ably-js v2.

However, there doesn't seem to have been much appetite so far for making a spec change (ably/specification#156 hasn't gained any traction). So, as @SimonWoolf suggested in that PR, let's tweak the typings just in ably-js, perhaps inspired by the approach of that PR.

I do not yet know exactly what form the final result will take.
@sync-by-unito
Copy link

sync-by-unito bot commented Oct 25, 2023

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3916

@lawrence-forooghian lawrence-forooghian mentioned this issue Oct 25, 2023
Open
@lawrence-forooghian lawrence-forooghian self-assigned this Nov 22, 2023
@lawrence-forooghian lawrence-forooghian mentioned this issue Nov 23, 2023
Merged
lawrence-forooghian added a commit that referenced this issue Nov 29, 2023
@lawrence-forooghian
Tighten type of publishing methods
19a8d47 
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
Copy link
Collaborator Author

Closed in #1515.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lawrence-forooghian lawrence-forooghian

Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@lawrence-forooghian