-
Notifications
You must be signed in to change notification settings - Fork 55
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
[ECO-1780] Removes broken feature matrix link #1689
base: main
Are you sure you want to change the base?
Conversation
README.md
Outdated
@@ -8,7 +8,7 @@ _[Ably](https://ably.com) is the platform that powers synchronized digital exper | |||
|
|||
This is a JavaScript client library for Ably Realtime. | |||
|
|||
This library currently targets the [Ably client library features spec](https://www.ably.com/docs/client-lib-development-guide/features/) Version 1.2. You can jump to the '[Known Limitations](#known-limitations)' section to see the features this client library does not yet support or [view our client library SDKs feature support matrix](https://www.ably.com/download/sdk-feature-support-matrix) to see the list of all the available features. | |||
This library currently targets the [Ably client library features spec](https://www.ably.com/docs/client-lib-development-guide/features/) Version 1.2. You can jump to the '[Known Limitations](#known-limitations)' section to see the features this client library does not yet support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This library currently targets the Ably client library features spec Version 1.2
oh, this also not true 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be spec version 3 or protocol version 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ttypic @umair-ably ping, so we can decide what we need to change here to resolve the conversation as we would like to merge this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for nudge, let's rephrase with protocol version 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The protocol page itself is a bit sparse, so I've left the link to the features page but mentioned it is with protocol version 2 - #4037953
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I think we should completely get rid of this paragraph that talks about the features spec or protocol version. Bear in mind that this README is quite conceivably the first point of contact that a user will have with our SDK. In that situation I’d be quite confused to be immediately confronted with a link to some internal development document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@umair-ably @lawrence-forooghian
I agree with Lawrence on this one. At least for the last couple of months we've been actively avoiding adding mentions of the "spec" to public documentation for ably-js (whether it is READMEs, changelogs or migration guides) based on the argumentation Lawrence has provided:
this README is quite conceivably the first point of contact that a user will have with our SDK. ... I’d be quite confused to be immediately confronted with a link to some internal development document.
As of now, mentioning the spec, protocol version or providing a link to the broken feature matrix doesn't provide anything of value to the user and quite frankly can confuse people even more.
So as Lawrence suggested, it might be a good idea just to remove this paragraph altogether from all SDKs for the time being. Once we introduce feature tracking, that will provide much better information to our users about capabilities of the particular SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go this route, my only question would be: do we, as Ably, actually utilize the information from this line, This library currently targets the [Ably client library features spec](https://www.ably.com/docs/client-lib-development-guide/features/) Version 1.2
? Do we use it as a source of truth to have a quick look at the the SDK and identify which protocol version (and, by extension, features) this particular SDK has implemented?
It seems like we do not, given that earlier in the thread Eugene mentioned that ably-js is actually targeting protocol version 2, not 1.2 - hence it's been outdated for some time.
If we do utilize this information in some way, then we should probably move it to a place other than the README, away from public view. Something like having a .ably-protocol-version
file in the repository root might suffice, containing a simple number inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have not yet come up with good language to use about which versions of various things a given SDK "targets". The best candidate, I think, is to talk about which specification version it targets (which, amongst other things, implies a certain protocol version). And, yes, that would be useful information for developers of the SDK to have. But I’d suggest that, for now, if that information is going to go anywhere, it should go in the CONTRIBUTING
document.
References Protocol Version 2
Removes broken feature matrix link - will be replaced in the future with an updated feature checklist.