-
-
Notifications
You must be signed in to change notification settings - Fork 864
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
Fixing ResolveObject API and unit tests #1713
Conversation
let res = search_by_apub_id(&self.q, local_user_view, context) | ||
.await | ||
.map_err(|_| ApiError::err("couldnt_find_object"))?; | ||
Ok(res) |
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.
We needed a proper API error message.
#[derive(Serialize, Debug)] | ||
pub enum ResolveObjectResponse { | ||
Comment(CommentView), | ||
Post(PostView), | ||
Community(CommunityView), | ||
Person(PersonViewSafe), | ||
#[derive(Serialize, Default)] | ||
pub struct ResolveObjectResponse { | ||
pub comment: Option<CommentView>, | ||
pub post: Option<PostView>, | ||
pub community: Option<CommunityView>, | ||
pub person: Option<PersonViewSafe>, |
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 needed to be a well-typed JSON object, no enums allowed, and rust doesn't allow union types ( probably for good reason ).
This is verbose, but very explicit on the receiving side. The only other alternative that might work is using a generic struct, but that leaves the API much less clear between the back and front ends.
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.
As far as I understand, an enum is essentially how Rust handles unions. And I dont understand why it isnt well-typed json. in terms of type safety it seems much better because you cant get an unexpected null value. Is it just that the struct is easier to parse from typescript?
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.
Rust can serialize enums, but they aren't JSON, which doesn't have that concept, only strings, numbers, arrays, all key value. AFAIK serde just takes enums and converts them to strings, but that leaves it really difficult to parse on the receiving side, because you then have to explicitly pick out fields that aren't in other ones to differentiate the types.
This is much more explicit and doable not just from typescript, but any language without having to deal with string enums.
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.
There are a few different ways that enums can be tagged by serde. Maybe "internally tagged" would work? (it basically adds a field "type" = "Comment"
.
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.
Lets just go with what I did here. I've already written it into the lemmy js client, and we have no other enums in the api anywhere.
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.
Hmm, I think the enum is a lot cleaner, and it will be very difficult to change later. I dont think its a good argument that we dont have other enums in the API, there is always a first time (and there might be other places where an enum would be cleaner).
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.
Enums are not json, and no client can read them. Typescript needs well typed objects.
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.
To get more specific, most languages don't have the concept of enum objects like rust does. Typescript only can do string and number enums: https://www.typescriptlang.org/docs/handbook/enums.html
If you look at serdes externally tagged enum code, Rust actually takes that enum object, and turns it into a key value where the key is a string of the enum name as the type. Its deserializer does the work of checking that type. Neither are actually enums, as json does't support them. This representation would only work if you deal only with rust, not with other languages who have no concept of enum objects.
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.
Okay then, one more reason for me to hate javascript/typescript.
b198ebf
to
251e0d3
Compare
767cc58
to
c525879
Compare
No description provided.