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

🧪 [Experiment] DependencyPropertyGenerator #624

Open
wants to merge 203 commits into
base: main
Choose a base branch
from

Conversation

Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Dec 28, 2024

Closes #621

This PR introduces a new DependencyPropertyGenerator component that utilizes source generators on partial properties to generate DependencyProperty quickly in your projects. This is the final API shape available with this PR:

namespace CommunityToolkit.WinUI;

[AttributeUsage(AttributeTargets.Property, AllowMultiple = false, Inherited = false)]
public sealed class GeneratedDependencyPropertyAttribute : Attribute
{
    public object? DefaultValue { get; init; }

    [DisallowNull]
    public string? DefaultValueCallback { get; init; }

    public bool IsLocalCacheEnabled { get; init; }

    [DisallowNull]
    public Type? PropertyType { get; init; }
}

public sealed class GeneratedDependencyProperty
{
    public const object UnsetValue;
}

This went through some community discussion before the current API design was finalized.

This PR includes:

  • The thin API surface with the attribute
  • A source generator for the attribute
  • A comprehensive set of analyzers
  • A code fixer to easily update all trivially supported dependency projects in a project/solution

Arlodotexe and others added 30 commits December 2, 2024 16:55
Copy link

@Lamparter Lamparter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking amazing ❤️

@Arlodotexe
Copy link
Member

Arlodotexe commented Jan 8, 2025

The completeness and stability of the code here inclines me to suggest including it in the mainline repo for 8.2, rather than housing the component in Labs. Labs is a parallel repo where we do incubation and new stuff until stable. Nearing 200 commits, this new component seems stable and well-tested.

@Sergio0694
Copy link
Member Author

It's well-tested in the sense that I have a ton of unit tests, but it's not really stable. It basically has 0 use in production. I fully expect that stuff might come up to warrant changing the API surface a bit or altering the generated code. Doing that would be a breaking change. I think it'd make sense to let this incubate a bit so that we can be sure stuff doesn't come up 🤔

@michael-hawker
Copy link
Member

Yeah, seems like we should just merge this into Labs and get feedback on this here? That's its purpose.

@Arlodotexe
Copy link
Member

Arlodotexe commented Jan 13, 2025

Understood. If we expect that the API surface might change, then testing production via Labs before including it in 8.x stable makes sense.

@michael-hawker
Copy link
Member

@Sergio0694 I assume we think the API is relatively good at this point.

Whenever we get the next clean build, it'd be nice to merge and get more eyes on things in a way other folks can use in the Labs feed. We can continue to make improvements in Labs, that's the point of it.

Comment on lines +5 to +8
<DependencyPropertyGeneratorUseWindowsUIXaml Condition="'$(DependencyPropertyGeneratorUseWindowsUIXaml)' == '' AND '$(TargetPlatformIdentifier)' == 'UAP'">true</DependencyPropertyGeneratorUseWindowsUIXaml>
<DependencyPropertyGeneratorUseWindowsUIXaml Condition="'$(DependencyPropertyGeneratorUseWindowsUIXaml)' == '' AND '$(UseUwp)' == 'true'">true</DependencyPropertyGeneratorUseWindowsUIXaml>
<DependencyPropertyGeneratorUseWindowsUIXaml Condition="'$(DependencyPropertyGeneratorUseWindowsUIXaml)' == ''">false</DependencyPropertyGeneratorUseWindowsUIXaml>
</PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having this be a bool, would it be better to have specific states? Like could we ever differentiate between WinUI 3 and WPF? In a similar to your check for "UseUwp"? Just wondering if we can generalize this at all in the future?

Copy link
Member Author

@Sergio0694 Sergio0694 Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would introduce complexity and I don't really plan on ever adding WPF support for this 😅

Copy link
Member

@Arlodotexe Arlodotexe Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-hawker There are some subtle differences between the DP system in traditional WPF and the
DP system in UWP/WASDK, but this generator wasn't build with those differences in mind. If it were, we might consider supporting traditional WPF in the future, but it would require significant planning that hasn't happened yet.

The primary target here was WinUI, same as all components that use our tooling. Nothing else in the Windows Community Toolkit supports WPF, outside of the WinUI-based MultiTarget afforded to us by Uno. Being WinUI-based, this generator already supports those targets.

Adding traditional WPF in any capacity would stretch both the generator code and our tooling beyond what they're currently designed to do.

@Sergio0694
Copy link
Member Author

@michael-hawker yup, already on it 😄
I'm just working with Arlo to fix the CI, that's the only reason why this hasn't been merged yet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experiment 🧪 Used to track issues that are experiments (or their linked discussions)
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

🧪 [Experiment] DependencyPropertyGenerator
5 participants