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

Address impact of change to interface on extras field on Message #580

Closed
QuintinWillison opened this issue Jun 9, 2020 · 6 comments · Fixed by #581 or #595
Closed

Address impact of change to interface on extras field on Message #580

QuintinWillison opened this issue Jun 9, 2020 · 6 comments · Fixed by #581 or #595
Assignees
Labels
bug Something isn't working. It's clear that this does need to be fixed.

Comments

@QuintinWillison
Copy link
Contributor

For 1.2.0 we changed Message.extras from JsonObject to MessageExtras.

This was a breaking change which we acknowledged as a TODO but never addressed.

We need to decide the approach for fixing this (either revert or extend the new MessageExtras class) in order to comply with TM2i.

@QuintinWillison QuintinWillison added bug Something isn't working. It's clear that this does need to be fixed. important labels Jun 9, 2020
@QuintinWillison QuintinWillison self-assigned this Jun 9, 2020
@paddybyers
Copy link
Member

Honestly I think extending MessageExtras is the way to go.

@mattheworiordan
Copy link
Member

mattheworiordan commented Jun 9, 2020

Why does this need to be a MessageExtras object when it's defined in the spec as JSON object?

In the spec:

extras JSON-encodable object, used to contain any arbitrary key value pairs which may also contain other primitive JSON types, JSON-encodable objects or JSON-encodable arrays. The extras field is provided to contain message metadata and/or ancillary payloads in support of specific functionality, e.g. push. Each of these supported extensions is documented separately; for 1.1 the only supported extension is push, via the extras.push member; 1.2 adds the delta extension which is of type DeltaExtras. The processing of any other members is undefined

If we go with a typed MessageExtras object, then every change we make in the protocol needs to be co-ordinated with an SDK change, which creates an unnecessary coupling and slows getting features to customers out.

@mattheworiordan
Copy link
Member

Related ably/docs#1639.

@SimonWoolf
Copy link
Member

SimonWoolf commented Jun 9, 2020

I have to agree with Matt - the spec item envisaged a json blob, opaque to the client library, so we could add new features without having to make painful client lib changes, or have to encode the knowledge of how all the push payloads are structured in each client lib, etc. I haven't seen anyone give a good reason for us to change that plan. (Of course typed languages might well want to parse extras.delta on incoming messages into a DeltaExtras object before interacting with it, but they can still do that)

@mattheworiordan
Copy link
Member

@QuintinWillison are there any other SDKs which have gone down this route in 1.2 and deviated from the spec?

@QuintinWillison
Copy link
Contributor Author

@mattheworiordan off topic for this issue and repository context. Moved to internal Slack conversation for the moment, until we have an explicit issue or issues to create in other repositories. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed.
Development

Successfully merging a pull request may close this issue.

4 participants