search

ably / specification

The Ably features spec for client library SDKs.
Apache License 2.0
0 stars 4 forks source link

Bump protocol version to 3 #173

Closed lawrence-forooghian closed 8 months ago

lawrence-forooghian commented 9 months ago

In order to get the flattened stats API (this change should have come in a731d12).

lawrence-forooghian commented 9 months ago

@SimonWoolf you mentioned on Slack that "you can request flat stats even with library protocol v2, if there's some particular reason you don't want to upgrade the lib protocol version yet, by specifying flatStats=true as a request param". I'm assuming that we'd prefer that client libraries bump the protocol version as I've done here, though?

SimonWoolf commented 9 months ago

if there's some particular reason you don't want to upgrade the lib protocol version yet, by specifying flatStats=true as a request param". I'm assuming that we'd prefer that client libraries bump the protocol version as I've done here, though?

sure -- only thing to bear in mind is that v=3 also triggers the new batched response, which means it's a breaking api change for anyone using rest.request() to do batched responses, which means that change should probably be made only in a library new major version, per semver. (But you'd probably want to do that anyway because completely changing the type of the rest.stats() response is also a breaking api change)

lawrence-forooghian commented 9 months ago

v=3 also triggers the new batched response

That probably means that alongside this protocol bump I should update the batch-related spec points to remove the requirement that they pass the newBatchResponse=true query param, right?

which means it's a breaking api change for anyone using rest.request() to do batched responses

I think this is no longer an issue now that Rest.request() requires the user to provide a version parameter (RSC19f1)? (I mean, that is itself a breaking change which is going into the same spec version as this PR targets, so it's kinda moot)

lawrence-forooghian commented 9 months ago

That probably means that alongside this protocol bump I should update the batch-related spec points to remove the requirement that they pass the newBatchResponse=true query param, right?

(I'll need to merge main into integration/3.0.0 to pull in the batch stuff first before I can do this, if one of you could approve #174 that would be great)

SimonWoolf commented 9 months ago

That probably means that alongside this protocol bump I should update the batch-related spec points to remove the requirement that they pass the newBatchResponse=true query param, right?

sure go for it

I think this is no longer an issue now that Rest.request() requires the user to provide a version parameter (RSC19f1)? (I mean, that is itself a breaking change which is going into the same spec version as this PR targets, so it's kinda moot)

yeah exactly, it fixes this problem for future breaking changes, but can't really do it retrospectively

(I'll need to merge main into integration/3.0.0 to pull in the batch stuff first before I can do this, if one of you could approve https://github.com/ably/specification/pull/174 that would be great)

I've changed the protection rules, you shouldn't need a pr review for a routine rebase of an integration branch, you can now self-review

lawrence-forooghian commented 9 months ago

I've pushed a new commit for the batch changes, mind re-reviewing please @owenpearson?