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

riot uses unstable APIs #5325

Closed
richvdh opened this issue Oct 18, 2017 · 12 comments · Fixed by matrix-org/matrix-js-sdk#990
Closed

riot uses unstable APIs #5325

richvdh opened this issue Oct 18, 2017 · 12 comments · Fixed by matrix-org/matrix-js-sdk#990
Assignees
Labels
P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect T-Task Tasks for the team like planning

Comments

@richvdh
Copy link
Member

richvdh commented Oct 18, 2017

There are a whole bunch of places in the js-sdk which call apis under the /unstable/ prefix; these are in turn used by riot-web. The ENTIRE POINT of unstable is to denote APIs which you can't rely on because they might change.

So WTF are we doing using them in released versions of riot?

@richvdh
Copy link
Member Author

richvdh commented Oct 18, 2017

This should be considered a release-blocker imho

@krombel
Copy link
Contributor

krombel commented Oct 26, 2017

Currently there are some endpoints that are not exposed by synapse on /r0/ but on /unstable/ only.
This issue is blocked on that missing ability.

@richvdh
Copy link
Member Author

richvdh commented Oct 26, 2017

yup. could you clarify what they are?

@dbkr
Copy link
Member

dbkr commented Oct 30, 2017

Here's a list of things known to be using the /unstable prefix in the js-sdk:

  • /login/cas/redirect (getCasLoginUrl)
  • /account/deactivate
  • /account/3pid/delete
  • /devices
  • /devices/$device_id (PUT and DELETE)
  • /keys/upload/$deviceId
  • /keys/query
  • /keys/claim
  • /keys/changes
  • /sendToDevice/$eventType/$txnId
  • /thirdparty/protocols
  • /thirdparty/location/$protocol

It looks like APIs that are in synapse unstable but not r0 are:

  • /account/3pid/delete
  • /login/saml2
  • /login/cas/redirect
  • /login/cas/ticket
  • /createUser

In other words, I think the answer to the question above is the CAS endpoints and 3pid deletion.

@richvdh
Copy link
Member Author

richvdh commented Nov 6, 2017

In other words, I think the answer to the question above is the CAS endpoints and 3pid deletion.

This is only true as of matrix-org/synapse#2579 (which has not yet been released). Before that I think that all of the things in the top list (except possibly /account/deactivate ?) were unstable-only.

@lampholder lampholder added this to the 0.15 - candidates milestone Mar 13, 2018
@lampholder
Copy link
Member

If supporting old versions of riot is something we've committed to do, then releasing versions of riot dependent on unstable APIs effectively commits us to supporting those unstable APIs in their current shape (rendering them stable by accident and not by choice).

Which is obviously terrible.

But, if we have already released those versions of Riot, then I don't see the particular need to block 0.14 on this issue. Certainly it's a high priority techincal debt maintenance item that needs to be resolved, and certainly each released version of Riot with these dependencies only makes the number of human users dependent on those unstable APIs greater which is undoubtably a BadThing.

But since this problem wouldn't be resolved by simply fixing our /unstable dependencies in 0.14, I'm going to bias towards this not being a release-blocker for 0.14. I will moot it for consideration in 0.15, though.

@richvdh I hope that makes sense - if you want to fist fight me on this we can schedule some time tomorrow 😛

@lampholder lampholder added T-Defect T-Task Tasks for the team like planning P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround and removed X-Release-Blocker labels Mar 13, 2018
@richvdh
Copy link
Member Author

richvdh commented Mar 13, 2018

fine, but continuing to punt it will result in toy-throwing tantrums.

@richvdh
Copy link
Member Author

richvdh commented Mar 13, 2018

It also uses /media/v1 for some of the media apis, which is also incorrect.

@lampholder lampholder modified the milestones: 0.15 - candidates, 0.16 - candidates Mar 26, 2018
@lampholder lampholder modified the milestones: RW008 - Candidates, RW009 - Candidates Apr 25, 2018
@richvdh
Copy link
Member Author

richvdh commented May 14, 2018

Be warned that my toys are nearing the edge of the pram again.

I think I might drop support for the unstable prefix in the next version of synapse. These APIs are unstable, so I can do that, right?

@ara4n
Copy link
Member

ara4n commented Feb 8, 2019

@lampholder can we get this on the radar for after riot 1.0 please?

@lampholder
Copy link
Member

/me has made a note

@turt2live
Copy link
Member

matrix-org/matrix-js-sdk#990 for the people who have a vested interest in Riot behaving itself as an API user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants