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

feature: Update Splat.Autofac to work with Autofac 5.0 (#465) #585

Merged
merged 8 commits into from
Sep 21, 2020
Merged

feature: Update Splat.Autofac to work with Autofac 5.0 (#465) #585

merged 8 commits into from
Sep 21, 2020

Conversation

HrlecMatej
Copy link
Contributor

@HrlecMatej HrlecMatej commented Sep 18, 2020

Raises Autofac version to 5.x in Splat.Autofac, and deprecates or removes methods that mutate the container (since that is supposed to be immutable from now on).
Register method is to be used only by the ReactiveUI initialization procedure, which modifies the container by creating a child scope for each service registration.

What kind of change does this PR introduce?

feature: Update Splat.Autofac to work with Autofac 5.x #465

What is the current behavior?

Splat.Autofac is currently locked to at most 4.9.6 version.

What is the new behavior?

Autofac dependency now targets version 5.x
It now (mostly) conforms to Autofac 5 container immutability.

What might this PR break?
Setup procedure is different. Before, everything was done on the IContainer (after the application's container has been built). Now (since containers are not mutable anymore), part of the setup happens earlier, on the ContainerBuilder.
Before:
`/* After the ContainerBuilder has been built */
container.UseAutofacDependencyResolver();

Locator.CurrentMutable.InitializeSplat();

Locator.CurrentMutable.InitializeReactiveUI();`

Now:
`/* While we still have the ContainerBuilder */
// Creates and sets the Autofac resolver as the Locator
var autofacResolver = containerBuilder.UseAutofacDependencyResolver();

// Register the resolver in Autofac so it can be later resolved
containerBuilder.RegisterInstance(autofacResolver);

// Initialize ReactiveUI components
autofacResolver.InitializeReactiveUI();

/* After the ContainerBuilder has been built */
var autofacResolver = container.Resolve<AutofacDependencyResolver>();

// Set a lifetime scope (either the root or any of the child ones) to Autofac resolver
autofacResolver.SetLifetimeScope(container);`

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)

I would appreciate if further testing could be done. I used this with a rather big application I am developing, and nothing broke. This of course does not mean, that no mistakes have been made.

Other information:

Raises Autofac version to 5+, and deprecates or removes method that mutate the container (since that is supposed to be immutable from now on).
Register and HasRegister are to be used only by the ReactiveUI initialization procedure, which modifies the container by creating a child scope for each service registration.
Thanks to "skaman" for coming up with the workaround.
@HrlecMatej HrlecMatej requested a review from a team September 18, 2020 09:29
@glennawatson
Copy link
Contributor

If you can update the readme.md file in the autofac directory with the new instructions that would be great.

@glennawatson
Copy link
Contributor

I'll merge this in AM my time to give time for the CI etc to work, and release later in the week.

@HrlecMatej
Copy link
Contributor Author

I'll try to fix up the tests as well

@glennawatson
Copy link
Contributor

Thanks

@HrlecMatej
Copy link
Contributor Author

HrlecMatej commented Sep 18, 2020

I will try to add reworked Unregister methods back in, in case somebody really needs them.
And add guards against trying to mutate the ContainerBuilder after an application's container has been built and SetLifetimeScope called.

@glennawatson
Copy link
Contributor

No problem. Just let me know when you are ready for a full review and merge.

@HrlecMatej
Copy link
Contributor Author

One pitfall about Unregistered. It's a bit hacky, since unregistering a service requires creating a new ContainerBuilder.
That means, one would have to pass the application's ContainerBuilder in like this: new AutofacDependencyResolver(ref containerBuilder).

@glennawatson
Copy link
Contributor

There's a similar approach with Microsoft di. We don't predict users will use it much and is more for native splat di.

@HrlecMatej
Copy link
Contributor Author

HrlecMatej commented Sep 18, 2020

Well, Splat doesn't need the Unregistered methods, that's why I skipped them at first. At least, I haven't found any reference (neither did anything break).

@glennawatson
Copy link
Contributor

There are cases where's it's needed. Mostly due to InitializeReactiveUI forcing dependencies on users in certain scenarios.

@glennawatson
Copy link
Contributor

There will be eventually a refactor of the init but it will allow users to work until that happens.

@glennawatson
Copy link
Contributor

Also there are initialization di hooks that will register containers. Like avalonia uses those for example

@HrlecMatej
Copy link
Contributor Author

Ok then, I'll do my best

@HrlecMatej
Copy link
Contributor Author

HrlecMatej commented Sep 18, 2020

Just realized about one big issue. Building containers will cause the following to fire: AutoActivate, Start on Autofac.IStartable and any RegisterBuildCallbacks.
And building the container is necessary, since that is the only way to get ComponentRegistry of all registered services, which is needed to repopulate a new builder in Unregister methods...

@HrlecMatej
Copy link
Contributor Author

HrlecMatej commented Sep 18, 2020

I added the check against trying to register new services to the dependency resolver, once the container has been built.

But I don't have any good idea how to solve the Unregistered methods issue, since firing post build callbacks is not an acceptable side-effect. Could you perhaps point me to where I could find calls to UnregisterCurrent/UnregisterAll ?

@glennawatson
Copy link
Contributor

You can forgo unregister just make a readme comment about it.

@glennawatson
Copy link
Contributor

It's only used by users if they don't want a view locator for example rxui uses. The users I know who are using it are on the main splat dictionary DI container.

Copy link
Member

@RLittlesII RLittlesII 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 submitting this!

src/Splat.Autofac/Splat.Autofac.csproj Outdated Show resolved Hide resolved
src/Splat.Autofac/README.md Show resolved Hide resolved
src/Splat.Autofac/AutofacDependencyResolver.cs Outdated Show resolved Hide resolved
src/Splat.Autofac/AutofacDependencyResolver.cs Outdated Show resolved Hide resolved
@HrlecMatej
Copy link
Contributor Author

HrlecMatej commented Sep 19, 2020

It's only used by users if they don't want a view locator for example rxui uses. The users I know who are using it are on the main splat dictionary DI container.

I am also using a custom ViewLocator (I am using it to resolve Views in the same lifetime scope as its ViewModel with Autofac), and my solution is simple.
Register your custom ViewLocator after InitializeReactiveUI:
autofacResolver.InitializeReactiveUI();
builder.RegisterType<MyCustomViewLocator>().As<IViewLocator>().SingleInstance();

Since Autofac's order of registration is deterministic, injecting IViewLocator will always return the custom ViewLocator.

@glennawatson
Copy link
Contributor

Yeah, so it negates the need I guess with autofac so we can just leave those as firing a not-implemented exception.

@HrlecMatej
Copy link
Contributor Author

HrlecMatej commented Sep 21, 2020

Yeah, so it negates the need I guess with autofac so we can just leave those as firing a not-implemented exception.

Another way registering custom services (e.g. ViewLocator) could be solved is by calling HasRegistration on this line, similar to how InitializeSplat handles that.

With this approach, one would register a custom service before the initialization call (like you currently do to override e.g. ILogger in InitializeSplat):

// Register your overrides first
containerBuilder.RegisterType<MyCustomViewLocator>().As<IViewLocator>().SingleInstance();
// Then call InitializeReactiveUI, which would ignore already registered IViewLocator
autofacResolver.InitializeReactiveUI();

This means there aren't any redundant registrations, and no potential unexpected side-effects from that.

But we can still just leave it be, since there shouldn't be any issues.

@glennawatson glennawatson merged commit 7af765b into reactiveui:main Sep 21, 2020
@glennawatson
Copy link
Contributor

If you make a issue in rxui it can be looked at in future versions.

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants