search

CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.87k stars 1.38k forks source link

Inconsistent naming for singleton properties (Shared|Default|Instance) #3493

Open Sergio0694 opened 3 years ago

Sergio0694 commented 3 years ago

Describe the issue

We have a bunch of singleton properties in the toolkit, but with inconsistent naming. I'm wondering whether it wouldn't make sense to have them all converge on a single term, to make them more intuitive for developers. Changing the name would obviously be a breaking change, so this seems like a good moment to consider this with 7.0 around the corner.

A few examples:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/a7f897616ba9b7d4ac26934f3439335be7830eb4/Microsoft.Toolkit.Uwp/Helpers/SystemInformation.cs#L40

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/32656db1cdd4ddc25e3a88c297ce4062fe64d2ad/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs#L89

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/1739fb8195f2479772747328c2a755df6b987761/Microsoft.Toolkit.HighPerformance/Buffers/StringPool.cs#L166

For reference, here's the singleton instance ArrayPool<T>.Shared from CoreCLR: link.

Expected behavior

Given that in all these cases we're dealing with singleton instances that are also thread-safe, hence having a similar role (with the given differences due to them being in different types), I figure maybe a single name should be used in all cases. To be consistent with CoreCLR too and since all instances do share the thread-safety feature too, I'm thinking probably Shared?

StringPool will definitely need to use Shared to be consistent with ArrayPool<T>.Shared, so the other two would need to be changed in that case. Otherwise, we could just leave the names different in those cases, looking for feedbacks on this 😄

Additional notes

A decision on this should be made before the 7.0 release is published (including #3230), and I was thinking maybe it'd be best to decide on this before #3424 is merged so that the updated name would already be included in the next preview package for the MVVM Toolkit, so that devs would have more time to get used to that?

Note regarding switching to Shared for messenger, that'd be different from Default being used in MvvmLight, but not necessarily an issue since devs moving from there would already have to make some code changes anyway.

cc. @michael-hawker @jamesmcroft

ghost commented 3 years ago

Hello Sergio0694, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

michael-hawker commented 3 years ago

I see the argument for all the current names, they're all slightly different cases. I think Instance is the original we started with which is kind of the standard for the pattern?

I mean I see developers grabbing the instance of SystemInformation more than them regularly going to grab the other ones?

So, I'm not sure if I see this as a big problem if we want to keep them separate. It also helps for the messengers to not lead developers to think they should be going to go grab the messenger instance and start using it, eh?

@azchohfi thoughts?

Sergio0694 commented 3 years ago

Yeah that's a fair point about SystemInformation using Instance, also because that class is mostly just a collection of info rather than a service-like instance actually doing work on its own like the others (especially eg. ArrayPool<T>).

I was mostly thinking about the Default for the messengers, and wondering whether there it wouldn't be clearer to use the same Shared name as the ArrayPool<T> in the BCL to underline the fact that the messengers are also thread-safe? 🤔

azchohfi commented 3 years ago

If we change anything, we should deprecate, instruct with a clear warning message, and then delete in a future version. Lets not directly remove any property which would cause a big breaking change and frustration to devs. I don't mind that much, but agree that consistency would be good. Now... for which one we should pick, that is a complex call hehe I like Instance way more than Shared, but this is a complete personal view.

Sergio0694 commented 3 years ago

"for which one we should pick, that is a complex call hehe"

Ah, naming being the most difficult problem in software engineering strikes back 🤣

If we don't want to deprecate/break things here (which is a good point I can agree with!), I guess then the remaining question for now would mostly just be about what to use for Default in the messengers? As in, whether to leave that and have a 3rd different name, or to switch them to Shared to be at least in line with ArrayPool<T> and StringPool and signal that thread-safety thing?

michael-hawker commented 3 years ago

@mrlacey any thoughts on this topic here? (as it relates in part to the theme of #3422)

mrlacey commented 3 years ago

It seems to me that Instance, Shared, and Default are all different (but related) things.

Instance - A singleton "instance" of a type. Shared - A collection used by multiple sources with a name that reflects that used within the CLR. Default - A default implementation of a type. Which may or may not be used as a singleton.

Having done a brief check of the code (master & 7.0preview4 branches) it looks like these are all being used as above. I don't think there's a need for any change here.

Sergio0694 commented 3 years ago

I'm a bit confused by two of the definitions you used there, could you clarify a bit? In particular:

"Shared - A collection used by multiple sources with a name that reflects that used within the CLR."

What do you mean by "collection of multiple sources"? It seems to me that eg. ArrayPool<T>.Shared basically matches your definition of "a default implementation of a type" (as it's a custom internal class implementing ArrayPool<T>), with the bonus point of being thread-safe, which is made explicit by the property name ("Shared" instead of "Default").

"Default - A default implementation of a type. Which may or may not be used as a singleton."

By this definition, shouldn't WeakReferenceMessenger.Default (and the strong one too) be called "Instance" instead? Since they're not the default implementation of a type, they're just singleton instances of those types, which are also sealed classes (so there is no concept of "default implementation" in this case). As in, it seems to me that "Default" according to your definition would be more appropriate for eg. an abstract class exposing an instance of some internal implementation? 🤔

mrlacey commented 3 years ago

Yes "Shared" is a special version of 'Instance' that reflects naming used within the CLR. It would be called "Instance" if it wasn't reflecting the CLR precedent for "pools" or "shared collections".

By this definition, shouldn't WeakReferenceMessenger.Default (and the strong one too) be called "Instance" instead?

Oops, yes. I was still under the old assumption that WeakReferenceMessenger.Default was an example of how to create an IMessenger implementation that uses weak refences. When in effect it's an IMessenger implementation that just happens to use weak references. The name tricked me into thinking there were other WeakReferenceMessenger instances, s if it was the default implementation or version of an IWeakReferenceMessenger. Changing to Instance would be clearer here.

Sergio0694 commented 3 years ago

"Yes "Shared" is a special version of 'Instance' that reflects naming used within the CLR. It would be called "Instance" if it wasn't reflecting the CLR precedent for "pools" or "shared collections"."

Ooh ok, I get what you mean, thanks!

"I was still under the old assumption that WeakReferenceMessenger.Default was an example of how to create an IMessenger implementation that uses weak refences. When in effect it's an IMessenger implementation that just happens to use weak references."

Had to re-read this like 3 times to be sure I was following, but yeah 😄 I think the Default name is a remnant from MVVMLight, which had a single Messenger type which was the "default" implementation of IMessenger, whereas we don't have one, but instead offer two different types implementing IMessenger, which as you said use either strong references or weak references, and act as singleton properties for their respective types.

cc. @michael-hawker if we all agree I can go ahead and rename to Instance in #3562 🙂