Skip to content

Conversation

@worldbeater
Copy link
Contributor

@worldbeater worldbeater commented Mar 5, 2021

What kind of change does this PR introduce?

With this PR, the InitializeReactiveUI extension method is now falling back to PlatformRegistrationManager.NamespacesToRegister if there are no registration namespaces provided via params of the InitializeReactiveUI method. The NamespacesToRegister property is mutable as opposed to DefaultRegistrationNamespaces — if we prefer NamespacesToRegister over the read only DefaultRegistrationNamespaces as the default value here, our users will be able to disable implicit dependency discovery if they'd like to via a call to PlatformRegistrationManager.SetRegistrationNamespaces().

What is the current behavior?

Currently, folks with the "Break on all exceptions" feature enabled in their IDE are confused by exceptions during app initialization:
System.IO.FileNotFoundException: Could not load file or assembly 'ReactiveUI.XamForms, Version=13.1.0.0, Culture=neutral, PublicKeyToken=null'. The system cannot find the file specified.

We know, that this is ordinary ReactiveUI dependency registration behavior — ReactiveUI tries to load assemblies in order to register the required services, and if the framework fails to load an assembly, then a FileNotFoundException is thrown, which is immediately caught by the MemoizingMRUCache implementation:

try
{
return Assembly.Load(assemblyName);
}
catch
{
return null;
}

This behavior is OK on most platforms, but there are folks who'd like to register the required dependencies manually and explicitly. That's why the SetRegistrationNamespaces method currently exists. At the moment we allow setting the registration namespaces, but don't allow turning off the auto dependency registration via assembly loading completely. Turning off this feature might be useful in such environments as Avalonia.ReactiveUI. Avalonia folks tend to use a IAppBuilder and the .UseReactiveUI() extension method for Avalonia-specific dependency registrations. That's why they might want to turn off the implicit ReactiveUI default dependency registration behavior.

What is the new behavior?

With the proposed change, one can disable implicit assembly loading by doing this:

PlatformRegistrationManager.SetRegistrationNamespaces(new RegistrationNamespace[0]);

This change would allow adding this statement to Avalonia.ReactiveUI in order to avoid performing unnecessary assembly loading in their environment. Also, this feature might be useful for folks who tend to use the "Break on all exceptions" feature in Microsoft Visual Studio or JetBrains Rider, and who might want to register the dependencies manually. In case if one disables platform registrations this way, the InitializeReactiveUI extension method will only register default registrations and .NET Standard/.NET Core etc. platform registrations.

What might this PR break?

Hopefully nothing, but let's wait for the CI?

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #2699 (6a19916) into main (de82224) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2699   +/-   ##
=======================================
  Coverage   63.57%   63.57%           
=======================================
  Files         132      132           
  Lines        4733     4733           
=======================================
  Hits         3009     3009           
  Misses       1724     1724           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de82224...6a19916. Read the comment docs.

@glennawatson
Copy link
Contributor

Wonder if a "Avalonia" enum might be better or something just because the empty array is a little meh. And on Avalonia just don't do anything.

Not quite sure if that's what we want, will get others opinion.

@RLittlesII
Copy link
Member

Yeah, returning nothing instead of the default could be a problem. I agree with Glenn that if Avalonia is specific, then we should open it up so that just Avaolnia returns an empty list.

@worldbeater
Copy link
Contributor Author

Thanks for the review! Added a RegistrationNamespace.Avalonia that isn't present in the possibleNamespaces dictionary and hence that platform just won't do anything. This means that on Avalonia.ReactiveUI side in the .UseReactiveUI() extension method we will just do:

PlatformRegistrationManager.SetRegistrationNamespaces(RegistrationNamespace.Avalonia);

And then register the Avalonia-specific dependencies manually.

@worldbeater
Copy link
Contributor Author

Going to fix the approval tests soon.

@worldbeater worldbeater changed the title fix: Use NamespacesToRegister instead of DefaultRegistrationNamespaces fix: Add RegistrationNamespace.Avalonia Mar 5, 2021
@worldbeater worldbeater marked this pull request as ready for review March 5, 2021 09:55
@worldbeater worldbeater merged commit 5d76936 into main Mar 5, 2021
@worldbeater worldbeater deleted the try-use-namespaces-to-register branch March 5, 2021 16:27
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants