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

Using TryResolve instead of Resolve with Autofac #413

Merged
merged 1 commit into from Oct 3, 2019
Merged

Using TryResolve instead of Resolve with Autofac #413

merged 1 commit into from Oct 3, 2019

Conversation

StanleyGoldman
Copy link
Contributor

What kind of change does this PR introduce?
Fixing AutofacDependencyResolver so it does not throw, catch and swallow exceptions while resolving services.

What is the current behavior?
I'm currently using AutofacDependencyResolver with ViewModelViewHost in a ListView and noticed the massive lag while debugging and the bloat of ComponentNotRegisteredExceptions being thrown even though my code works.

What is the new behavior?
The new behavior should be the same, except faster when using Autofac and debugging.

What might this PR break?
Nothing I can think of.

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:

@dnfclas
Copy link

dnfclas commented Oct 3, 2019

CLA assistant check
All CLA requirements met.

@StanleyGoldman StanleyGoldman marked this pull request as ready for review October 3, 2019 13:44
@StanleyGoldman StanleyGoldman requested a review from a team October 3, 2019 13:44
@StanleyGoldman StanleyGoldman changed the title Using TryResolve instead of Resolve Using TryResolve instead of Resolve with Autofac Oct 3, 2019
@glennawatson
Copy link
Contributor

I think @RLittlesII wanted to give some feedback on this PR so hold on before merging.

@dpvreony dpvreony merged commit 7c0698c into reactiveui:master Oct 3, 2019
@dpvreony
Copy link
Member

dpvreony commented Oct 3, 2019

gah that poped up right as I did it :(

@glennawatson
Copy link
Contributor

Ah well, I'm sure @RLittlesII can put the feedback and provide another PR if necessary.

@RLittlesII
Copy link
Member

RLittlesII commented Oct 3, 2019

@glennawatson @dpvreony

I was honestly wondering if the performance issue was because the exception is thrown.

The documentation states that TryResolve will still throw the DependencyResolutionException so I am not sure this change will actually resolve the issue.

Both ResolveOptional() and TryResolve() revolve around the conditional nature of a specific service being registered. If the service is registered, resolution will be attempted. If resolution fails (e.g., due to lack of a dependency being registered), you will still get a DependencyResolutionException. If you need conditional resolution around a service where the condition is based on whether or not the service can successfully resolve, wrap the Resolve() call with a try/catch block.

https://autofaccn.readthedocs.io/en/latest/resolve/

@StanleyGoldman
Copy link
Contributor Author

I keep on re-reading it and I feel like the part of the documentation contradicts what was stated and demonstrated in the section right above it...

// If IService is registered, it will be resolved; if
// it isn't registered, the return value will be null.
var service = scope.ResolveOptional<IService>();

// If IProvider is registered, the provider variable
// will hold the value; otherwise you can take some
// other action.
IProvider provider = null;
if(scope.TryResolve<IProvider>(out provider))
{
  // Do something with the resolved provider value.
}

@StanleyGoldman
Copy link
Contributor Author

Nope, I think I understand what the documentation is saying..
The document is saying that if you think the "construction" is going to fail, you will still need to try/catch
"construction" != "resolution"

@lock lock bot locked and limited conversation to collaborators Jan 1, 2020
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.

None yet

5 participants