NewInstance: register default instance before new instance (fixes #197) #198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…get(Dep)` I started with some tests that were more similar to my actual use case before realizing the difference between the working tests and the failing case. The distilled failing test case here is: `new instance of a dependency does not become the default instance in the container` For the existing tests, swapping the order of getting App1 and Logger causes all of the NewInstance tests to fail.
krisdages
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#197
(Edit)
I looked into the implementation and found what I think is a reasonable fix to keep the default singleton behavior for undecorated injections while still allowing new instances to be added to an array strategy:
If there is no resolver for the NewInstance key, auto-register the key before calling
container.registerInstancewith the new instance.I'm only calling autoRegister if the new instance key is a function; i'm not sure exactly what the expected behavior would be if the key is a primitive. There is also a type assertion that the key is a
constructor; makes me a little uncomfortable, but I see that same type assertion elsewhere in the
container, so I think it's consistent in that regard.
(Original): Failing tests only
I started by adding some tests that were more similar to my actual use case
before realizing the actual difference between the working tests and the failing case.
The distilled failing test case here is:
new instance of a dependency does not become the default instance in the containerFor the existing tests, swapping the order of getting App1 and Logger causes all of the NewInstance tests to fail.
PR is just for info purposes unless you want to add these extra tests.Also, my editor kept changing line endings to CRLF, realized the wrong value was in .editorconfig