CommunityToolkit / Maui

The .NET MAUI Community Toolkit is a community-created library that contains .NET MAUI Extensions, Advanced UI/UX Controls, and Behaviors to help make your life as a .NET MAUI developer easier
https://learn.microsoft.com/dotnet/communitytoolkit/maui
MIT License
2.09k stars 337 forks source link

[BUG] [Breaking Change] - Custom behaviors derived from BaseBehavior<TView> no longer compile #1822

Closed bengavin closed 1 month ago

bengavin commented 1 month ago

Is there an existing issue for this?

Did you read the "Reporting a bug" section on Contributing file?

Current Behavior

As of 8.x, inheriting from BaseBehavior for custom behaviors no longer compiles. This worked correctly as of the 7.x build.

Expected Behavior

Compilation succeeds when inheriting from BaseBehavior

Steps To Reproduce

  1. Create a new custom behavior, inheriting from BaseBehavior [e.g. BaseBehavior]
  2. Try to compile
  3. Notice the error:
    CS0122  'BaseBehavior<Entry>.BaseBehavior()' is inaccessible due to its protection level

Link to public reproduction project repository

N/A (will add if truly needed, but this is a simple compile error)

Environment

- .NET MAUI CommunityToolkit: 8.0.1
- OS: Windows
- .NET MAUI: 8.0.20

Anything else?

If the base class is not intended to be used outside the library, it feels like that should have been listed as a breaking change in this release and the preferred alternative documented.

bijington commented 1 month ago

@brminnick this appears to be the breaking change: https://github.com/CommunityToolkit/Maui/pull/1791/files#diff-db7d97856c23232f65ddfea7a5df4eed07e5c4fcaaa45e6f6f8cc7c9b1b91555 was it intentional? I am not convinced it was but keen to find out before I undo it.

UPDATE I have opened PR #1823 in case we are happy

brminnick commented 1 month ago

Yes, it was done intentionally as BaseBehavior is undocumented and not intended to be used outside of CommunityToolkit.Maui.

Could you help me understand your need to inherit from BaseBehavior?

bengavin commented 1 month ago

Yes, it was done intentionally as BaseBehavior is undocumented and not intended to be used outside of CommunityToolkit.Maui.

Could you help me understand your need to inherit from BaseBehavior?

I've re-aligned to Behavior as a workaround for this issue. The why was a consistency thing for us, since we're inheriting from the toolkit's base converters and whatnot, and the BaseBehavior was available, it was assumed safe to just derive from that. Looking at the existing toolkit behaviors as an example for our implementation also played into it. :)

Looking into the code when troubleshooting this earlier, I noticed that there isn't anything happening in there we need today, and I'm unsure if there is a benefit to having the community toolkit interface tied into our custom behavior over the long term.

bengavin commented 1 month ago

I guess I should follow-on with noting that the various base converters we are using are also apparently undocumented, is the expectation that these should NOT be used as base classes?

brminnick commented 1 month ago

Looking into the code when troubleshooting this earlier, I noticed that there isn't anything happening in there we need today, and I'm unsure if there is a benefit to having the community toolkit interface tied into our custom behavior over the long term.

Thanks for confirming!

brminnick commented 1 month ago

base converters we are using are also apparently undocumented, is the expectation that these should NOT be used as base classes?

Correct - same as BaseBehavior, BaseConverter is an internal helper class. You're welcome to use it, but it is unsupported and undocumented and, like with BaseBehavior, may lead to breaking/unexpected changes in the future.

bengavin commented 1 month ago

base converters we are using are also apparently undocumented, is the expectation that these should NOT be used as base classes?

Correct - same as BaseBehavior, BaseConverter is an internal helper class. You're welcome to use it, but it is unsupported and undocumented and, like with BaseBehavior, may lead to breaking/unexpected changes in the future.

Is there any particular reason for this? The base converters, at least, provide convenient type-safe support for one and two way type conversion. It feels like a missed opportunity to require/suggest folks re-implement [copy] that into their own base converters, rather than relying on the well tested and widely used classes already provided in the toolkit?

brminnick commented 1 month ago

Is there any particular reason for this?

Time + Resources, mostly. We are all volunteers and none of us get paid for our work on this project. We must be judicious with our time and deliver features with the highest value.

You're welcome to do the work if you'd like:

  1. Submit a New Feature Proposal
  2. Once the New Feature Proposal is approved, submit a Pull Request in this repository
    • Add in additional Unit Tests covering all possible edge cases
    • Update the XML comments to improve intellisense
  3. Submit a Pull Request on the Docs repo documenting the new feature and adding examples of how to use it
  4. Maintain the new feature
    • Respond to Issues submitted
    • Verify Issues using the provided reproduction samples
    • Submit a Pull Request fixing the Issue, also including Unit Tests that cover the fixed bug to avoid regressions
bengavin commented 1 month ago

Fair enough :)

gabsamples6 commented 1 month ago

@brminnick we totally appreciate the work you guys are doing but you have broken our app in multiple places as we were using the base behaviour of the toolkit to create additional behaviors that suits our need. Even though you might have a case I dont think you should have made it private after so long it was public.

pictos commented 1 month ago

@bengavin @gabsamples6 , the fast fix for it is importing the BaseBehavior's code to your code base and use it. Since it's OSS and MIT you can do it without any trouble.

gabsamples6 commented 1 month ago

@pictos thanks for replying that is exactly what we did - no choice - again we appreciate the time you dedicate to this project!!!!