feat: WIP refactor of lowest level CAC layer #168
Conversation
| @@ -103,7 +93,7 @@ public async Task<ColorPalette[]> GetAllAvatarColors(string avatarId) | |||
| var response = await authorizedRequest.SendRequest<Response>( | |||
| new RequestData | |||
| { | |||
| Url = AvatarEndpoints.GetColorEndpoint(avatarId), | |||
| Url = $"{Endpoints.API_V2_BASE_URL}avatars/{avatarId}/colors?type=skin,beard,hair,eyebrow", | |||
There was a problem hiding this comment.
This is the crux of point 3, and if used everywhere will negate the need for the endpoints classes which we can then remove.
There was a problem hiding this comment.
While I do agree it is kinda weird that we have an endpoint class, I don't really know of a better way to do it unless we can isolate their usage into their own classes or something (no need to share across scripts). In general I feel like the endpoints are all over the place so it's hard to group them together in a way that makes sense.
There was a problem hiding this comment.
How do you mean? This proposed change would remove the need for them, right (other than the base one?). Where is the endpoint declaration needed other than the methods which make the API requests?
IMO the base one should become a environment file (like a .env, or an appsettings.json in .net APIs) and would then be more common place. I also have a cool idea how to implement that in a way to make it easy to switch between dev env and prod env.
| @@ -93,8 +82,9 @@ public async Task<AvatarProperties> CreateFromTemplateAvatar(string templateId, | |||
|
|
|||
| response.ThrowIfError(); | |||
|
|
|||
| var json = JObject.Parse(response.Text); | |||
| var data = json[DATA]!.ToString(); | |||
| var createDraftAvatarResponse = JsonConvert.DeserializeObject<CreateDraftAvatarResponse>(response.Text); | |||
There was a problem hiding this comment.
Example of point 1.
There was a problem hiding this comment.
I do personally like the idea of using Json Deserialize object as it is what I am used to and I find it to be more readable, I think it might also have some minor perfomance benefits particularly when dealing with large json payloads. However my main concern with it is that the data responses change so much with the web API's quite often a new property is release before we even hear about it (an example of that was modular asset categories some months ago).
I don't care too much about having to make classes for the responses but one thing that is annoying is that the data responses from different endpoints are really inconsistent with their naming which might cause us to make extra classes just because they use iconUrl instead of imageUrl or something 😅 (this is kinda the same as Robins point)
In saying that though probably these things are more stable nowadays and probably less likely to change as time goes on.
There was a problem hiding this comment.
Another thing that kinda comes to mind as a downside of Deserialize approach is that some of our data responses have quite a nested structure which would mean we need to make more classes to hold the data
There was a problem hiding this comment.
Another thing that kinda comes to mind as a downside of Deserialize approach is that some of our data responses have quite a nested structure which would mean we need to make more classes to hold the data
IMO this is actually a positive thing, and not a downside. The SDK then becomes a reference for developers to understand the types returned by the API, much like other more mature SDKs do. Like the Stripe SDK for example.
I don't care too much about having to make classes for the responses but one thing that is annoying is that the data responses from different endpoints are really inconsistent with their naming which might cause us to make extra classes just because they use iconUrl instead of imageUrl or something
It'd be standard practice for basically every endpoint to have it's own response class, even if there is a lot of overlap between them in some endpoints. It's sort of a ball-ache at first, but then it should be fine once done. If there are any things at the API level which don't make sense then this would enable us to find them and report them, rather than just cover them up 😅
However my main concern with it is that the data responses change so much with the web API's quite often a new property is release before we even hear about it (an example of that was modular asset categories some months ago).
JsonConvert.Deserialize can be setup to have loose mapping though, which means if properties disappear then it won't throw, it'll just not populate them, and if new properties are added then they'll just get ignored at first until the types are updated. Obviously it would be the most ideal to have the APIs be more stable, but I don't really see how using JsonConvert would be any different here to the JObject approach we have now? 🤔
| @@ -0,0 +1,16 @@ | |||
| using System.Collections.Generic; | |||
|
|
|||
| namespace ReadyPlayerMe.AvatarCreator.Avatars | |||
There was a problem hiding this comment.
Example of class output of using point 1.
|
I disagree with point 1 as it leads to the generation of unnecessary classes that are only utilized at a single point. Additionally, I see no issue with employing constants in a file; this practice aligns with fundamental programming principles in Unity or C#. Even Rider recommends this approach. |
|
I regret to differ on the choice of naming the web request "avatarApi." In my perspective, "avatarApi" implies a server-side API, potentially leading to ambiguity in the future. In Unity, we commonly favor terms like "request," as exemplified by UnityWebRequest for REST APIs. While I concur with renaming "partnerAssetRequest" to "AssetApiRequest," I find the term "ImageApi" misleading. There isn't a corresponding backend "imageApi"; rather, it pertains to a web request for downloading images. It might be more fitting as an extension or utility function for image fetching. Actually there is already one, but it seems I forgot to use it 😅 |
I tend to agree with Robin about the class naming thing. It's kinda nitpicky but I do find AvatarApi a little misleading but it also kinda feels like it's against the Unity conventions. Unity devs are quite used to the whole "XXXXXRequest" naming convention when it comes to making web requests. |
|
Also about point 3, I don't think having a generic environment file is a good approach, at least externally (partner/developer facaing). Internally it's fine but if we try use it for other developers using our SDK we will have lots of headaches around ensuring the file exists, forcing it's creation, and making sure that devs don't delete it or move it and stuff. It's quite similar to the difficulties we still have right now with the CoreSettings scriptable object file.😅 |
You guys didn't read me initial message fully 😄 - I'm fully happy with it not being called Api (although Api doesn't just mean backend FYI, like low level mobile APIs for example), but either way the suffix itself isn't what I'm concerned about, it's more than I would like to realise the benefits of it being uniform and consistent with {Resource}(that matches the API resource){Suffix}. If you guys know Requests to be the most common in Unity then I'm happy with that. |
What makes them unnecessary? Is not just having type declarations alone is a benefit to developers who can then do with the SDK whatever they wish? It's a lot more self documenting than what we have right now, and makes it easier for developers to understand how the SDK works, and what options they have in terms of changes they make, even to the lowest level. |
For what reason would it not be a good approach? It would exactly the same as our current Endpoints.cs, except it would be called Environment.cs? It would still have hardcoded values in the a cs file for everyone so what would be different in terms of it being more negative? |
|
Closing this PR as the changes we're moving forward with have been broken down into separate smaller PRs 😄 |
This is a half complete PR to demonstrate a few potential refactors I'd like to make of our lowest level layer in the custom avatar creator. I haven't changed everything yet, as I would like to discuss what's here first before spending much time on wider changes. The main things in this PR which I would like people to view are:
Scrapping the JObject JSON parsing, and instead use JsonConvert.DeserializeObject (still Newtonsoft) so that we can get strong typing of response classes, and don't have to have hard coded constants at the top of files to pull out particular properties. See the first few methods of AvatarApi for reference.
Having a uniform naming for our lowest level api wrappers. Currently it is a mix of 'AvatarAPIRequests, PartnerAssetRequests, and so on'. Regardless of whether the suffix is requests or api or something else, I would like it to be uniform to {Resource}{Suffix}.
Scrapping the concept of endpoint classes which are only used in one place. Instead we would have one file where the base urls are derived from (I'd like this to become a generic environment file in the future), and then the resourceApi methods will each be responsible for defining the url they call. This means these lowest level methods are fully encapsulated with no unnecessary references to another file.
Some small tidy ups too, like replacing the bespoke icon downloader with a more reusable ImageApi class containing an image downloader.
Please leave your thoughts and comments.
Note: not everything is changed and this isn't a working PR right now so it will break if you pull it down, but I wanted to illustrate these ideas rather than just talk about them