umbraco / Umbraco.Headless.Client.Net

.NET Client Library and Samples for the Umbraco headless cms Cloud Service called Umbraco Heartcore
https://umbraco.com/products/umbraco-heartcore/
13 stars 14 forks source link

Feature request: Easier ways to get Content as T #11

Open NiclasLindqvist opened 4 years ago

NiclasLindqvist commented 4 years ago

When requesting content of a specific type, the inferred documentType name is nice as long as types are simple, but when my class name doesn't match the documentType things get unnecessarily hard. Please add string contentType to GetByType<T> and other <T> methods so that the developer can get whatever documents they want and still treat it as T in their context.

Also, a way to convert IContent to a T of your liking would be nice (automatically resolving the Properties dictionary to their corresponding class properties)

sitereactor commented 4 years ago

Thanks for the suggestion, makes perfect sense. We could probably add it as an overload option.

For the IContent to T part is it helper method you are after? And why not just request as T instead of converting afterwards?

NiclasLindqvist commented 4 years ago

For IContent to T a helper would be nice. One reason for it is the before mentioned, getting content not named like the T, or getting multiple documents of different types using GetDecendants and sorting out what contentType they are after the fact. Maybe I’m using it wrong but I end up needing to get Content and making them into T later. 🙈

rasmusjp commented 4 years ago

Thats a very good point and it got me thinking that we might be able to make it even nicer than having to call a mapping method.

What I'm thinking is to auto deserialize the results into the custom model types.

The custom model types could be registered in the configuration

var config = new HeadlessConfiguration();
config.ContentModelTypes.Add<TextPage>();
config.ContentModelTypes.Add<BlogPost>();
...
var client = new ContentDeliveryService(config);

and then when deserializing the models we use the type list to resolve the correct model type

So if you call GetDescendants() without using the generic overload the result returned would be of IEnumerable<IContent> and the items would be of the types that matches what's registered (E.g. [TextPage,BlogPost]), and then you can then cast each item to the specific model.

For the name/alias resolving we add a custom attribute that can be added to the models e.g.

[ContentModel("textPage")]
public class UnrelatedModelName : IContent {
}

[MediaModel("Image")]
public class Image : IMedia {
}

and if the attribute isn't there we resolve the alias from the type name as we do today.

sitereactor commented 4 years ago

@NiclasLindqvist your initial suggestion with being able to provide the doc type alias via a string is available in this RC release on nuget if you want to try it out. https://www.nuget.org/packages/Umbraco.Headless.Client.Net/1.1.0.31470-RC

New method is called GetByTypeAlias

NiclasLindqvist commented 4 years ago

Hi @sitereactor, sorry for getting back on this so late, the holidays got in between. I saw the GetByTypeAlias method on master now, looks nice!

Is it possible, through the API, to get all children of a specific type? I'm thinking that "GetChildren" would get all children of a parent, that are of a specific type. And if so, it would be handy to have GetChildrenAlias where contentTypeAlias is a parameter as well. As it stands right now, I think GetChildren will get all children and treat them as T even though the document is of a different type.

sitereactor commented 4 years ago

Hi @NiclasLindqvist that part is what Rasmus has implemented in this PR: https://github.com/umbraco/Umbraco.Headless.Client.Net/pull/12

I haven't had a chance to review it yet, but you are welcome to have a look if you'd like. It would be interesting to get an extra set of eyes on it and let us know if you have any feedback/input.

nul800sebastiaan commented 4 years ago

We currently can't prioritize getting this properly tested but we'll get to it eventually. If you have some more feedback on this Niclas then we'd be happy for an update!

NiclasLindqvist commented 4 years ago

I think #16 and #12 together solves this issue very nicely, I've been running my project based on #12 since early january, and can't think how i'd sovled my project without it. So from my perspective, it should be merged, any issues arising from this will not be more severe than the issues I've ran in to with the client and/or heartcore already.

sitereactor commented 4 years ago

Just to elaborate on this a bit more. The reason why we are hesitant to merge the PR is because we want to test it in an Xamarin/iOS/Android setup to ensure that we don't break compatibility and doing these tests takes quite a bit of time.

I agree that we should have this and we want to add it, why just need to be sure we don't break stuff as we merge this (mostly around the reflection parts).

martinjt commented 3 years ago

What's the sort of timeline on this? We really need this functionality to take on Heartcore. This is pre-requiste to getting a pipeline in ASP.NET core that allows views to be rendered dynamically.

I'm happy to take this out of this repo and write our own if the timescale is long.

sitereactor commented 3 years ago

The PR which addresses the better type handling has been merged now. We'll do a few more tweaks before releasing it, as we want to change how the https status is handled by refit so it doesn't throw exceptions - and ideally add a Web library as well. I'll post back with a link to an updated package when its released. Would expect it to be released later this week.