-
-
Notifications
You must be signed in to change notification settings - Fork 917
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
Remove api v3 routes. #5508 #5516
base: main
Are you sure you want to change the base?
Conversation
@@ -183,7 +183,7 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { | |||
// Site | |||
.service( | |||
scope("/site") | |||
.route("", get().to(get_site_v4)) | |||
.route("", get().to(get_site)) |
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.
On L181
I still kept this under api/v4
, but I'm not sure we really need it. Seems like apps / clients will have to use version in nodeinfo or coming back from GetSite
anyway, to decide which API version to use.
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'm not sure we really need it
What exactly do you mean?
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.
Instead of serving these under /api/v4
, we could just serve them under /api
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.
People might create simple scripts that use the api without checking nodeinfo
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 want to make any breaking changes in the future it will be much easier with a versioned api. So its better to keep it, rather than remove and then add it back later.
Lets wait with this until the new api is actually supported by apps (#5508). Api v3 should still work for basic browsing until then. |
At the moment, certain v3 endpoints (e.g. search) return v4 responses on Lemmy 1.0. You say that the v3 API should continue to work for "basic browsing" - does this exclude support for those currently broken v3 endpoints? |
I would say that either we should add a compat layer so the v3 endpoints return (mostly) the same structure as before, OR do as #5508 says and wait until most apps support the v4 api cleanly. I don't think it's obvious which option is better. Adding a compat layer is some dev effort but I think it would be simple (?). Waiting for app support requires less dev work but much more coordinating effort. Personally I think adding a compat layer for v3 would be better, though I only saw surface level what the structural changes are. |
Again, it's deceptive to say that the existing v3 routes in
Even if we had a much larger dev team, it'd still be a ton of work, definitely not doable right now. |
Really? Isn't it just ~3-7 structs (PostView, CommunityView, ...) that you'd need to create a wrapper for? If it can't be made "mostly" compatible, the routes should definitely be removed. Otherwise it's just confusing. |
This diff contains all the type and API changes. |
A while ago with the 1.0.0-alpha.0 tag, basic functionality like login, post listing and comment listing were still working with api v3. However this is probably not the case anymore once #5429 and #5515 are merged. Creating backwards compatible structs would be a huge amount of work, not to mention the effort to keep it maintained for the entire lifetime of Lemmy 1.x versions. So that is not feasible. |
It doesn't need to be forever - I don't think removing an API that has been marked deprecated for a while is an issue in a minor upgrade. The goal in the end is to encourage instances to upgrade without fear and to encourage apps to support the new version - they will have to support both for a while in any case. Regarding the effort - maybe it's effort, but I think it remains to be seen whether "get all major apps to support both versions of lemmy in a reasonable time" is less effort - as far as I understood the main thing that's changed is the API paths and the struct content - not the logic. A compat layer for the first is already here (by not applying the PR), and for the second it's "just" (AI-generatable) 1:1 translations. Anyways, I think we should only merge this PR once clear progress is made on #5508 (i.e. at least some apps have implemented or at least commented on the v4 API), there's not a large cost to keeping these files around for now? |
Seeing as most Lemmy apps are open source, I would rather make some pull requests to help them use the new API, rather than making more changes to Lemmy for some backwards compatibility layer. |
The main question is how we can handle the transition from 0.19 to 1.0, when some instances still use the old version, and others already use the new one. There will have to be some way to make apps work with both types of instances, at least in a limited way. Its best to discuss this directly with client devs, eg by opening issues in their git repos (#5508). It may also be worth to talk with them via Matrix chat about handling the upgrade. |
Apps should either use backwards-compatible API libraries like MV-GH's for kotlin, or pull down tagged libraries as git submodules (IE lemmy-js-client v0.19, v1.0, etc). Then they can build version specific components (or even better, stop supporting dead versions.) |
No description provided.