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

Auto-generate Lemmy-API types. #657

Merged
merged 24 commits into from
Jun 19, 2023
Merged

Auto-generate Lemmy-API types. #657

merged 24 commits into from
Jun 19, 2023

Conversation

dessalines
Copy link
Member

@dessalines dessalines commented Jun 14, 2023

Okay this took weeks, but its ready to go. Changes:

  • All the API types are now autogenerated from typescript (and those were autogenerated directly from rust). No more manually adding types!
    • I've done this so that adding Moderator actions #23 , as well as any lemmy actions, will be much easier.
    • Just run the ./copy_generated_types_from_lemmy_js_client.sh, where lemmy-js-client can be on any branch / API version you want to develop for.
    • If anyone knows how to create a kotlin library, so that we can externalize the HTTP client and its types, that would be much appreciated. I tried and failed using sonatype, as their documentation is very outdated.
  • The HTTP library nows uses a sealed kotlin class calledApiState, with 4 states: Empty, Loading, Success(data), and Failed(message). This makes handling Api states much more predictable, and catchable.

Notes:

  • The new types account for some breaking changes related to 0.18.0:
    • We plan to release Lemmy 0.18.0 sometime next week 🤞 , but you can test this version of jerboa using https://voyager.lemmy.ml .
    • The other big one, is that logins to 0.17 lemmy versions won't work. The user's default sort and listing types, as well as the API calls, actually use their enums now, and not strings. This is a breaking change that means users will not be able to log in with 0.17.0 versions. I'll create a release/v0.17 branch that people can cherry pick to, if they really need to test logins on old versions.
    • Another is that SiteRes.Taglines is no longer optional: if empty, it will always be an empty list, rather than non-existent. I've allowed this to fail softly in code.
    • How to handle breaking API changes is a question I could use yalls help on. Its quite a hassle to try to support multiple API types in one codebase, and querying for server versions, rather than just telling people they need to use older jerboa version to work with older lemmy software.

I had to deal with some pretty hairy merge conflicts with main, so I apologize if things got wiped out.

@dessalines dessalines marked this pull request as ready for review June 16, 2023 12:05
@dessalines dessalines requested a review from twizmwazin as a code owner June 16, 2023 12:05
@dessalines dessalines changed the title Autogen types merge 1 Auto-generate Lemmy-API types. Jun 16, 2023
@dessalines
Copy link
Member Author

@twizmwazin mmk tests are passing, apologies for CI issues again... woodpecker is supposed to do a release in a few days.

If there's anything you want me to describe, lmk, cause I realize this one's massive.

But it does need to get merged before any other ones do.

Copy link
Contributor

@beatgammit beatgammit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I left a few comments here and there, mostly for my own curiosity.

defaultListingType = form.default_listing_type ?: account.defaultListingType,
defaultSortType = form.default_sort_type ?: account.defaultSortType,
defaultListingType = form.default_listing_type?.ordinal ?: account.defaultListingType,
defaultSortType = form.default_sort_type ?.ordinal ?: account.defaultSortType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space before ?.ordinal, odd that the formatter didn't catch it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's weird, I got it now.

copy_generated_types_from_lemmy_js_client.sh Outdated Show resolved Hide resolved
@beatgammit
Copy link
Contributor

beatgammit commented Jun 16, 2023

Ok, I did some testing and found a crash here (mark an inbox message as read):

java.lang.NullPointerException: Attempt to invoke virtual method 'com.jerboa.datatypes.types.Comment com.jerboa.datatypes.types.CommentView.getComment()' on a null object reference

at com.jerboa.ui.components.inbox.InboxViewModel$markReplyAsRead$1.invokeSuspend(InboxViewModel.kt:355)

comment_view is null

Edit: The crash is because the server is responding with comment_reply_view and we've comment_view. Probably just a typo. Changing the return type in markCommentReplyAsRead fixes the issue.

Edit: #711 fixes this.

@beatgammit
Copy link
Contributor

beatgammit commented Jun 16, 2023

Related: when I reply to inbox messages, I can't enter text, but I can submit empty replies.

https://voyager.lemmy.ml/comment/151

I can reply to comments, so I can go to the link then reply there.

It looks like doing this lets me mark as read.

Edit: #715 fixes this.

dessalines and others added 4 commits June 19, 2023 07:27
The issue was caused by the activity checking for a completed request in
the PostViewModel. However, since this activity was triggered by the
inbox, that request was never made.

The only data it needed was whether the user was a moderator, so I pass
that information in with the deep link. In all cases we had enough
information in the navigating activity to determine this, with the
exception of the inbox, but we don't mark moderators in the inbox (at
least lemmy-ui doesn't).

This is a little bit of a hack, but it's a relatively clean hack.
@dessalines
Copy link
Member Author

This isn't perfect of course, and I'm sure we'll find more bugs that I introduced later on :(, but I'll get it merged as it needs to be here for 0.18.0 (hopefully released soon), and blocking other PRs as a result.

@dessalines dessalines merged commit 7d9c5a6 into main Jun 19, 2023
@dessalines
Copy link
Member Author

Special thanks to @beatgammit for finding and fixing these bugs.

@beatgammit
Copy link
Contributor

No worries, glad to see this get merged. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants