-
Notifications
You must be signed in to change notification settings - Fork 384
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
MSC1497: Advertising support of experimental features in the CS API #1497
Comments
Fixes #490 (from the looks of it) |
A couple of notes:
EDIT: strikethrough where I'm very wrong. UNEDIT: (point 1 no longer strikethrough) I don't think I was very wrong; at most slightly wrong. |
Unstable is mentioned throughout the spec. It's even listed as a possible version. |
It slightly surprises me that we don’t mention unstable anywhere, but not that much. Might as well define it now as a prefix for experimental new stuff which hasn’t been versioned and released in the spec yet.
Probably not, imo, in line with the antigoals section. The point of the monolithic version number of the CS API is to define the set of endpoints (and thus modules) which play form a single well defined release of the spec - even if some of those modules are optional.
If a module’s API needs backwards incompatible changes we’d just bump the overall version for the CS API to reflect it. Plus I’m hoping we might get the LL stuff right before it is baked into a release version number of the CS API.
Hm, this feels quite feature creepy to me...
I guess i was trying to keep the response size small and avoid redundant info. After all, optional modules should very much be the exception rather than the norm, and by the time they become compulsory the clients will assume they’re available and not care about the /versions response.
It’s feature creepy, but i agree it’d be really nice to fix that at last. |
e2e key backups seem like another possible use for this |
I actually really like the idea of having modules be versioned separately from the main spec, and I don't see it at all in the vein of XMPP fragmentation. The idea is more akin to how some PLs are now versioning their language separately from their standard library. We get a solid foundation of functionality from the main matrix spec (the general set of abstractions that matrix uses: rooms, users, events, devices, etc.) and then modules build convenience features on top of that. Since the modules are by definition not basic functionality of the matrix protocol (this is how it is now, too, not just in the hypothetical future I'm painting), it might be more necessary to iterate faster on the shape of those apis/conventions. Basically, I'm advocating for separate versioning of modules because of the following advantages I see:
Disadvantages: this requires some sort of negotiation of which version of modules is used by client/server. This requires servers to potentially maintain multiple versions of the same functionality. More care must be given to how modules are written to make sure that they can take part in whatever "module version handshake" is chosen above. This is discussing almost entirely a different proposal than the one written here (the reason I bring it up is that implementation of this proposal begins to preclude versioning of modules). I definitely see the disadvantages, but I'm really not super content with the current shape of lazy-loading and m.room.message, and I'd prefer not to have to wait until r2 (assuming r1 comes this month) to get those right.
Not including all of the fairly small number of modules because of response size feels like premature optimization to me (different matter if we were talking about |
What I'm getting at I guess is that nowhere on the C2S spec page does it say something like "Endpoints for versions with major version number x will be found under the |
It is though - it could potentially be made clearer, however the table on the index has a list of the current versions for the all the specs. Each spec then has a list of versions that are available. It looks like we do have a bug where the unstable c2s spec no longer has The intention is that by clicking on "unstable" (or HEAD as it's called for some reason) the endpoints reflect what version prefix to use. Likewise, if we ever hit an r1 then clicking on an r0 spec should show r0 for the endpoints. |
Okay that's fine in general (although I'd prefer an explicit statement of the organizing scheme rather than implicit), but endpoints under "unstable" are frequently implemented or changed before a spec PR has even landed to the unstable spec, so I feel like we should have a note somewhere about how endpoints under that prefix may be experimental (or some other similar verbiage). |
From the MSC:
This feels very complex and something that might be better handled by spelling it out rather than having implicit inheritance. With the proposed approach it is easy for a client to assume that r0.x will have lazy loading provided it knows how to speak r0.4 to the server. An older client would then not pick r2 as a viable option anyways because it wouldn't understand it, however a newer client might understand r2. With the client's previous assumption, it would be very possible that the client would try the r0.x lazy loading against r2 if the server didn't advertise it, as the client is trying to figure out how the versioning works. As mentioned, the solution could be explicitly listing supported versions. For the example scenario, lazy loading would have |
My main concern with letting module versions drift from spec versions is that there is going to be a lot of cross dependencies between versions of the spec and versions of the modules, in terms of implementing the features. I really want to avoid a soup of ifdefs on clients where it says things like "if we are on r0.5.0 of main spec and version r0.6.0 on the module, then we can do it like this...", and really, its just going to be a pain to implement, test and maintain client side. And it'll be fragile when a new server comes along and has a different mix of module version. There are very real benefits to having separate versions, but I also feel that we don't want to be bumping versions of modules that much more often than the main spec anyway, for exactly the same reasons. I vote at the very least that if your server supports an optional module it must support the same version as the latest version of the main spec the server supports, i.e. if your server supports |
I've updated the googledoc to reflect erik's feedback (and out-of-band feedback from travis & vdh), and propose a simpler solution for now: that we should pause on the question of negotiating optional modules (and thus whether optional modules should need their own version numbers, and what the semantics should be for servers which only implement modules at given versions of the spec), and instead simply advertise feature flags for experimental features which have not yet landed in the spec. Thus there's now a proposal for an Meanwhile, I think the whole question of how much to ratify "/unstable" as a version or prefix is slightly orthogonal to the question of cap neg, and eitherway can be punted for now in light of the simpler proposal. |
I think the "simpler proposal" is a good idea. I'm not sure after having slept on it if separately versioned modules are actually a good idea for matrix, which tells me the general design of "optional" features needs some more thought before given structure in the spec. |
Having a separate Three options spring to mind:
|
I was thinking of using option 3 if we ever hit that scenario for incompatible new experimental versions of an existing feature. |
I vote that this MSC should enter the final comment period with the view of having it be a spec PR next week. Can the Spec Core Team either approve going into FCP or comment with any concerns. Those outside the team with concerns/suggestions are also welcome to raise them. |
The watered down version that made it into synapse is working fine so far and is pretty uncontroversial. 👍from me. |
(that synapse PR being a subset of matrix-org/synapse#3589 ) |
👎. Please can the document be cleaned up to clearly show exactly what is going on. |
@richvdh agreed. Shouldn't take too long. |
I've fixed up the MSC. @richvdh, @anoadragon453 , @erikjohnston, PTAL again. |
I guess this has happened by default, because LL relies on it and it is implemented in synapse and riot. |
I'm renaming this to make way for an MSC for a feature which fits the 'capabilities' name better. |
Merged via #1786 🎉 |
Documentation: https://docs.google.com/document/d/10t3Ehz1JZWdDCSAS3SvxZZwb3cXYnJBoOPL4tcIKNYg/edit#heading=h.h8g8a1l2xtks
Sorry, I should have done this as a markdown MSC; consider it the last of a dying breed.
The text was updated successfully, but these errors were encountered: