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

ReadOnlyKernel breaks current use cases #176

Open
cr7pt0gr4ph7 opened this issue Jun 12, 2015 · 6 comments
Open

ReadOnlyKernel breaks current use cases #176

cr7pt0gr4ph7 opened this issue Jun 12, 2015 · 6 comments

Comments

@cr7pt0gr4ph7
Copy link

The introduction of IReadOnlyKernel, and the deprecation of IKernel, break a lot of current use cases of Ninject.

In my code, for example, I use Ninject for implementing an extension point system, where I can dynamically add extensions and extension points, while already having instanced some extensions that were registered before. This is only possible because Ninject allows to mutate the bindings and get instances at the same time. With KernelConfiguration and ReadOnlyKernel, this is not possible anymore, so I would be forced to switch to another DI container.

Even StandardKernel has to go to a lot of hoops to implement the previous functionality.
As an aside, I believe that it even doesn't implement the previous interface correctly, because calling StandardKernel.ReadonlyKernel creates a new kernel after configuration changes, and thus throws away currently activated instances. This is most likely not what one would expect when using a property.

Separately from that, I really liked the simplicity of Ninject regarding the interface of IKernel and the other parts. In my opinion, there is no need to introduce IReadOnlyKernel, because that interface already exists: It is called IResolutionRoot. IKernelConfiguration already exists too: It is called IBindingRoot. So I'm curious to know what the needs and use cases behind the switch to IReadOnlyKernel are.

Sorry if this sounds a bit like a rant; I'm just a little bit worried because I find Ninject v3 to be the one DI container around with the best usability, and I would like to continue to use it in my projects (home and company).

@cr7pt0gr4ph7
Copy link
Author

The only additional functionality of IReadOnlyKernel that I can see is that it allows to clone a "template" kernel, so that it is based on the same configuration but does not share the activated instances etc. But I can't imagine a real use case for that; neither one, which could not be solved using an IKernel.Clone() method.

@scott-xu
Copy link
Member

Please refer #84

@cr7pt0gr4ph7
Copy link
Author

Thank you for the pointer to #84! That answers my question of the "why".

I'm glad that using a readonly kernel is not set in stone, but instead only a mean to an end (that is, better activation performance), which could possibly be reached in another way. One way would be caching of the resolved bindings.

I'll add further discussion to #84, but I'm leaving this issue open, because, as it stands, it is a problem.

@omar omar mentioned this issue Dec 11, 2016
@scott-xu
Copy link
Member

ReadOnlyKernel has been backed out.

@drieseng
Copy link
Member

drieseng commented Mar 23, 2019

@scott-xu Now that ReadOnlyKernel has been re-introduced, can you please address the concerns raised by @cr7pt0gr4ph7 ?

@drieseng drieseng reopened this Mar 23, 2019
@drieseng
Copy link
Member

drieseng commented Mar 23, 2019

From my point of view, the main advantage of ReadOnlyKernel is that we do not need to lookup a component whenever we need one. I'll have to run some benchmarks to see how much this moves the needle.

There do appear to be some issues with this approach:

  • All (readonly) kernels built using KernelConfiguration.BuildReadOnlyKernel() share state (eg. cache) because they use the same set of components. Is this what we want, or do we expect users to build a given KernelConfiguration only once?

  • As @cr7pt0gr4ph7 pointed out, KernelBase and StandardKernel are broken since we - for example - lose all implicit bindings whenever the kernel is modified.

  • Deprecating and eventually removing IKernel and StandardKernel will be a source breaking change for all extensions. We're talking about a major release of Ninject here, so that by itself is not a big issue. Problem is that we do not provide a path forward for most of these extensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants