MicrosoftDocs / WindowsCommunityToolkitDocs

Creative Commons Attribution 4.0 International
179 stars 156 forks source link

Update Messenger.md #626

Closed zzfima closed 2 years ago

zzfima commented 2 years ago

Note about Architectural principle violation

Fixes #

Docs for Toolkit PR #

What changes to the docs does this PR provide?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

zzfima commented 2 years ago

Probably, my way of explain can be changed, English is my foreign language.

opbld30 commented 2 years ago

Docs Build status updates of commit b218a72:

:warning: Validation status: warnings

File Status Preview URL Details
docs/mvvm/Messenger.md :warning:Warning View Details
dotnet/xml/CommunityToolkit.Graph.Uwp.Converters/UserToPersonConverter.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.Graph.Uwp.Converters/ObjectToStringConverter.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.Graph.Uwp.Controls/PeoplePicker.xml :warning:Warning View Details
dotnet/xml/Microsoft.Toolkit.Uwp.UI.Controls/TokenizingTextBox.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/Carousel.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/BladeView.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/CarouselItem.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/DropShadowPanel.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/GridSplitter.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/DockPanel.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/EyedropperToolButton.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/ImageCropper.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/InfiniteCanvas.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/MenuItem.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/StaggeredPanel.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/Menu.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/UniformGrid.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls.TextToolbarButtons/DefaultButton.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/WrapPanel.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/RadialGauge.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/WrapLayout.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/TabbedCommandBarItemTemplateSelector.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/RadialGaugeAutomationPeer.xml :warning:Warning View Details
dotnet/xml/CommunityToolkit.WinUI.UI.Controls/TokenizingTextBox.xml :warning:Warning View Details

This comment lists only the first 25 files in the pull request.

docs/mvvm/Messenger.md

dotnet/xml/CommunityToolkit.Graph.Uwp.Converters/UserToPersonConverter.xml

dotnet/xml/CommunityToolkit.Graph.Uwp.Converters/ObjectToStringConverter.xml

dotnet/xml/CommunityToolkit.Graph.Uwp.Controls/PeoplePicker.xml

This comment lists only the first 25 errors (including error/warning/suggestion) in the pull request. For more details, please refer to the build report.

If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

zzfima commented 2 years ago

@Sergio0694 hi, can you approve/decline changes? Thanks

Sergio0694 commented 2 years ago

I'm not sure I understand how "WeakReferenceMessenger violates Explicit dependencies architectural principle". Could you elaborate and explain what you're referring to?

zzfima commented 2 years ago

I'm not sure I understand how "WeakReferenceMessenger violates Explicit dependencies architectural principle". Could you elaborate and explain what you're referring to?

When a function within its argument not implies some interface IXxx but inside of body subscribes by messenger to events of interface IXxx, IMHO it violates "Explicit dependencies" principle. Do not you think so? Thanks

Sergio0694 commented 2 years ago

I'm still not sure I understand what you mean. That link says the following:

"Methods and classes should explicitly require any collaborating objects they need in order to function correctly. [...]. If you define classes that can be constructed and called, but that will only function properly if certain global or infrastructure components are in place, these classes are being dishonest with their clients."

I don't see how that is the case for WeakReferenceMessenger. That type has no implicit dependencies, and it can be constructed and used on its own. It does not depend on some other global state or additional setup that needs to be done. You can instantiate it and then use those APIs it exposes (or pass or inject it as an IMessenger instance), and it will work just fine.

Can you please elaborate on what you mean here? Maybe with some additional context, an example or something? I would like to understand what you're trying to say here, but I need more details 🙂

zzfima commented 2 years ago

Hi @Sergio0694 The problem is, class not showing to out world, that it use some interface. I thought it's works against those principles, and this (from same link): "By following the explicit dependencies principle, your classes and methods are being honest with their clients about what they need in order to function. Following the principle makes your code more self-documenting and your coding contracts more user-friendly, since users will come to trust that as long as they provide what's required in the form of method or constructor parameters, the objects they're working with will behave correctly at run time."

Probably, I am wrong, but when some class, that I not aware of, got some message, it's alarms me

zzfima commented 2 years ago

@Sergio0694 or do i need to suggest text change to those Explicit dependencies? To write something, like: ....these classes are being dishonest with their clients... except when using Event Aggregator...

or not this and not this?

thanks!

Sergio0694 commented 2 years ago

I have to say I still don't understand what you mean here at all and how it relates to that explicit dependencies. I don't see how the messenger type is "being dishonest" with anyone, really, or how it has hidden dependencies.

@michael-hawker help? 😅

michael-hawker commented 2 years ago

Sorry for delay, missed ping on this.

@Sergio0694, I don't think he's saying that the WeakRefenceMessenger itself violates the Explicit dependencies principle. He's saying by using this in a class, your class now has a 'hidden ("dishonest") dependency" that anyone using that class doesn't know about.

It's important to note the article linked to is an excerpt from a book in relation to web and cloud technologies. The principle is basically saying there shouldn't be a case where your component in a vacuum fails because there was some unknown requirement needed by the client.

I can understand this principle being important for a public API or service; so that's important that someone knowing an API knows what side-effects its going to have or what it needs to function. But even then, some internal message pump is an implementation detail.

Usually messages are a notification for someone else to listen to and not a network message going back and forth required for something to function. The messenger pattern and this class is specifically designed for UI construction and coordination.

These are two different things related to different technologies. I don't think this is useful to call out at all and point a MVVM based library on UI to an excerpt of an azure/web doc about architecture.