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

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

Open
dr1rrb opened this issue May 27, 2024 Discussed in #2307 · 8 comments
Open

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

dr1rrb opened this issue May 27, 2024 Discussed in #2307 · 8 comments

Comments

@dr1rrb
Copy link
Member

dr1rrb commented May 27, 2024

Discussed in #2307

Result of investigation (with more details) can be found here : #2307 (reply in thread)


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

public sealed partial class MainPage : Page
{
    public MainPage()
    {
        this
            .Background(ThemeResource.Get<Brush>("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

internal partial record MainModel
{
    public IState<MachineModel> Machine => State.Value(this, () => new MachineModel("", ""));

    public ValueTask GetMachine() => Machine.UpdateAsync(m => m?.Load("100"));
}

MachineModel

internal partial record MachineModel(string Id, string Name)
{
    public MachineModel Load(string id)
    {
        return new MachineModel(id, $"Machine {id}");
    }
}

UnoMvux1.zip

@dr1rrb
Copy link
Member Author

dr1rrb commented May 27, 2024

The most appropriate solution would be to generate types for the "second generation kind" (cf. #2307 (reply in thread)) 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
Copy link
Contributor

francoistanguay commented Jul 12, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

dr1rrb commented Jul 12, 2024

@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 #2307 (reply in thread))

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
Copy link

mcNets commented Jul 12, 2024

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
Copy link
Contributor

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.

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

No branches or pull requests

4 participants