unoplatform / uno.extensions

Libraries to ease common developer tasks associated with building multi-platform mobile, desktop and web applications using Uno Platform or WinAppSDK.
https://platform.uno/
Other
73 stars 47 forks source link

[MVUX] Creating a `State<XXXModel>` break code gen if `XXXModel` is a `record` #2310

Open dr1rrb opened 5 months ago

dr1rrb commented 5 months ago

Discussed in https://github.com/unoplatform/uno.extensions/discussions/2307

Result of investigation (with more details) can be found here : https://github.com/unoplatform/uno.extensions/discussions/2307#discussioncomment-9573080


Originally posted by **mcNets** May 25, 2024 Testing C# Markup + MVUX, while trying to reproduce the Counter example, I encountered an error when adding the second partial record, the BindableViewModel is not accessible anymore. I tried this in two different projects. ``` Build started at 10:55... 1>------ Build started: Project: UnoMvux1, Configuration: Debug Any CPU ------ 1>Skipping analyzers to speed up the build. You can execute 'Build' or 'Rebuild' command to run analyzers. 1>CSC : warning CS8785: Generator 'FeedsGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'ArgumentException' with message 'The hintName 'UnoMvux1.MachineModel.g.cs' of the added source file must be unique within a generator. 1>c:\Test\UnoMvux1\UnoMvux1\MainPage.cs(9,30,9,47): error CS0246: The type or namespace name 'BindableMainModel' could not be found (are you missing a using directive or an assembly reference?) 1>Done building project "UnoMvux1.csproj" -- FAILED. ``` MainPage ```csharp public sealed partial class MainPage : Page { public MainPage() { this .Background(ThemeResource.Get("ApplicationPageBackgroundThemeBrush")) .DataContext(new BindableMainModel(), (page, vm) => page .Content(new StackPanel() .VerticalAlignment(VerticalAlignment.Center) .HorizontalAlignment(HorizontalAlignment.Center) .Children( new StackPanel() .Orientation(Orientation.Vertical) .Children( new TextBlock() .Text("Hello Uno Platform!") .FontSize(24) .Margin(10), new Button() .Content("Click me!") .Margin(10) ) ))); } } ``` MainModel ```csharp internal partial record MainModel { public IState Machine => State.Value(this, () => new MachineModel("", "")); public ValueTask GetMachine() => Machine.UpdateAsync(m => m?.Load("100")); } ``` MachineModel ```csharp internal partial record MachineModel(string Id, string Name) { public MachineModel Load(string id) { return new MachineModel(id, $"Machine {id}"); } } ``` [UnoMvux1.zip](https://github.com/unoplatform/uno/files/15442428/UnoMvux1.zip)
dr1rrb commented 5 months ago

The most appropriate solution would be to generate types for the "second generation kind" (cf. https://github.com/unoplatform/uno.extensions/discussions/2307#discussioncomment-9573080) into a different namespace (and into another file name).

Note: Currently even if we add the [Value] attribute on the State<MachineModel> Machine property, the "second generation kind" will still generate its BindableMachineModel but it will not been used ... we should also prevent that!

francoistanguay commented 3 months ago

For reference, this is what we're thinking in terms of updating naming conventions:

Given:

record Person; record PersonModel() { IState Person; } record OtherModel() { IState Person; }

For each model XYZModel, instead of generating BindableXYZModel, we would generate XYZViewModel.

For each unique IState, instead of generating BindableEntity, we would generate EntityState.

So you would end up here with 3 classes generated: PersonViewModel, OtherViewModel, PersonState

dansiegel commented 3 months ago

Conventions are great... Custom Conventions are better and Configurability should always be possible.

[MvuxSkip]
public record MyModel { }

[MvuxModel]
public record MyRecord { }

[assembly: MvuxConvention(Suffix = "Modelo")]

What we see in the above example is something where we can: 1) Opt out of Generation 2) Opt into Generation even though the record doesn't actually meet the convention 3) We are actually changing the convention to be localized for Spanish

francoistanguay commented 3 months ago

Sure, we could eventually add some configurability like for new Suffixes.

I'd be curious to see a case where you want to skip a class suffixed Model containing IFeed/IState public properties.

dansiegel commented 3 months ago

I could be mistaken... but I thought the whole point is that we have a convention for the Generation on Type name rather than looking for Records that have IFeed/IState. With any such convention then we're already ignoring types that may have IFeed/IState.

dr1rrb commented 3 months ago

@dansiegel You can use the [ReactiveBindable(false)] attribute on your types. You can also enable/disable/configure the "convention" using the [ImplicitBindables("MyApp\.Presentation\.\.*Model$")] attribute on your assembly. (It was suggested as workaround in the original bug report https://github.com/unoplatform/uno.extensions/discussions/2307#discussioncomment-9573080)

The issue is that there is still a bug in the handling of those attributes (even if we disable the generation for a model, we are generating the sub-bindable that would have been used by that model) + naming clash of generated types.

mcNets commented 3 months ago

I agree with @dansiegel , by default my models are basicaly a bunch of properties with no other functionality, maybe some calculated values. Even if you don't generate the Bindable (Reactive) part, I still need to notify property changes.

Besides:

[MvuxSkip]
[MvuxModel]
public record MyModel { }

you could add:

[MvuxObservable]
public record MyRecord{ }
francoistanguay commented 3 months ago

In MVUX, a Model is the record containing State. State gets exposed as IState If you dont need to update state, because it will only be reaodnly, you expose it as IFeed.

If you need INPC, that's because values can change, either when they're being loaded (thats what IFeed does) or because user made an action (that's what IState does).

So the convention is that your model is suffixed by Model and contains either IFeed and/or IState.

There's nothing less or more to it.