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

Container.resolve should resolve in that container #1338

Closed
tonyhallett opened this issue May 7, 2021 · 6 comments · Fixed by #1339
Closed

Container.resolve should resolve in that container #1338

tonyhallett opened this issue May 7, 2021 · 6 comments · Fixed by #1339

Comments

@tonyhallett
Copy link
Contributor

https://github.com/inversify/InversifyJS/blob/master/wiki/container_api.md#containerresolvetconstructor-interfacesnewablet-t

Resolve is like container.get(serviceIdentifier: ServiceIdentifier) but it allows users to create an instance even if no bindings have been declared:

It is like it but is resolved in the Context of a child container. This is not mentioned in the documentation and I would expect that most people would expect that this is just sugar for below

this.bind<T>(constructorFunction).toSelf();
const resolved =  this.get<T>(constructorFunction);

In most scenarios the difference is not important but it is if you are expecting context.container to be the container that you called resolved upon.

Soln - resolve from the same container !

@notaphplover
Copy link
Member

Hi @tonyhallett , I think a child container is being created to avoid adding a binding to the original container. I think users don't expect their bindings to be modified so I wonder if we should take this approach. What do you think @PodaruDragos , @dcavanagh ?

@tonyhallett
Copy link
Contributor Author

I think a child container is being created to avoid adding a binding to the original container.

Agreed - it is necessary for there to be no binding to be able to use resolve more than once.

The solution in the pull request - bind, get, unbind has the same behaviour as the current code.

The example I provided, with a derived container, is real but was mainly chosen for illustration purposes. Most importantly, the issue can manifest if we decide to change the internals of the Container class by setting some private property. At the moment there is no such problem as the child gets the options of the parent and the middleware is copied across.

@notaphplover
Copy link
Member

But if the user had already set a binding for that type we would be removing its binding, is that right?

@tonyhallett
Copy link
Contributor Author

tonyhallett commented May 9, 2021

That is true but is that the purpose of resolve ?

Resolve is like container.get(serviceIdentifier: ServiceIdentifier) but it allows users to create an instance even if no bindings have been declared:

Why would you bind and then want to avoid the resolution logic ?
Perhaps the binding could possibly have been done and you don't want to check isBound beforehand ?

You are right though, the solution does not have the same behaviour as the current code.
Perhaps a container.isBound test would then provide the same behaviour ( in the context of the description above )

@notaphplover
Copy link
Member

That is true but is that the purpose of resolve ?

Resolve is like container.get(serviceIdentifier: ServiceIdentifier) but it allows users to create an instance even if no bindings have been declared:

Why would you bind and then want to avoid the resolution logic ?
Perhaps the binding could possibly have been done and you don't want to check isBound beforehand ?

I think there's no good reason to perform the operation, but the operation could be performed and I think we should try to provide the right behavior

You are right though, the solution does not have the same behaviour as the current code.
Perhaps a container.isBound test would then provide the same behaviour ( in the context of the description above )

Yeah, that would work :)

@tonyhallett
Copy link
Contributor Author

Changed to only bind / unbind is not already bound.

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

Successfully merging a pull request may close this issue.

2 participants