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

Allow configuring current UI framework to avoid exceptions during dependency registrations #2396

Closed
wants to merge 2 commits into from

Conversation

JesperTreetop
Copy link

@JesperTreetop JesperTreetop commented Apr 27, 2020

What kind of change does this PR introduce?
A small feature: A configuration knob to cull the list of registration classes that dependency registration attempts to find, so as to avoid exceptions that are thrown and then caught in the background on every application launch, which has visible effects sometimes if first-chance exceptions are being caught. See #2395 for related discussion.

This is a short-term feature, and the problem will hopefully be addressed in greater detail by construction as part of init being improved (#1737).

What is the current behavior?
Speculatively load all registration classes in all assemblies.

What is the new behavior?
Have a list of "registration namespaces" which can be overridden by the user. The API is not ideal because the API in reality ends up being called from RxApp's static constructor. In real application usage, any parameters has to be passed out-of-band in some way by a method called before the RxApp static constructor and separate from the method where they are used, and so that is what the code does.

What might this PR break?
If setting the registration namespaces was mandatory it would break every app. The list defaults to all of them, which is backwards compatible with the current behavior.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:
I have added a test which verifies that specifying WPF and running the DependencyResolverMixin will work just the same. It does not check that other registration classes are excluded.

@JesperTreetop JesperTreetop requested a review from a team April 27, 2020 13:22
/// <summary>
/// Platforms or other registration namespaces for the dependency resolver to consider when initializing.
/// </summary>
public enum RegistrationNamespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer this to be a separate non-nested enum.

Copy link
Author

Choose a reason for hiding this comment

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

I considered that and didn't know in which file it would live - is a neighboring file okay or should it live in some other folder?

{
"ReactiveUI.XamForms",
Copy link
Contributor

Choose a reason for hiding this comment

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

The order we previously looked was used as part of our previous tests. It may be best to instead of using a dictionary to use a List. It's only 6 entries so a dictionary probably wouldn't perform better anyway.

Copy link
Contributor

@glennawatson glennawatson left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, apart from minor tweaks looks all good. The API approval test, it produces a ".received.txt" in the approval folder, if you copy/paste over the top of the ".approved.txt" file it will update it with the new API.

@JesperTreetop
Copy link
Author

It looks like I'll have to continue this in a bit. The full build and therefore the test don't run on my machine because I'm missing a lot of the frameworks, and the test did do something other than what I was expecting, so I have some more work to do before this is ready.

@JesperTreetop JesperTreetop marked this pull request as draft April 28, 2020 08:43
@glennawatson glennawatson changed the base branch from master to main June 8, 2020 05:02
@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 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants