-
Notifications
You must be signed in to change notification settings - Fork 169
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
Supports passing a default provider in the inject
decorator.
#264
base: master
Are you sure you want to change the base?
Supports passing a default provider in the inject
decorator.
#264
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
@@ -19,6 +24,12 @@ function inject( | |||
multiple: false, | |||
isOptional: options && options.isOptional | |||
}; | |||
|
|||
if (options && options.defaultProvider) { | |||
// @ts-expect-error options.defaultProvider is the right type but this Typescript version doesn't seem to realize that one of the overloads method would accept this type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider refactoring this section to remove the reliance on @ts-expect-error by either upgrading TypeScript or adjusting the overload definitions to properly type-check the defaultProvider.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
|
||
if (options && options.defaultProvider) { | ||
// @ts-expect-error options.defaultProvider is the right type but this Typescript version doesn't seem to realize that one of the overloads method would accept this type. | ||
globalContainer.register(token, options.defaultProvider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like defaultProvider
has the side-effect of making a registration for the token. I'm not sure how I feel about this.
Seems we could get into undefined behavior territory depending on the order things resolve if multiple things use default providers for the same token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very true. We almost want something like, on resolve, if there is no registration, then check to see if we have a provider in a list of default providers, that we can check into before throwing an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so this may need to go into the token descriptor, similar to isOptional
.
Though I wonder if we could get it working with ES6's default parameters syntax, since I feel most usages of this would be for default values as opposed to factories and the like.
(if we're registering a factory, we may as well have a full fledged registration; though I could be wrong and not seeing the right use case yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Factories, I could see wanting easily access to the container at runtime. Let me try to put this in the token descriptor to see if that makes more sense.
This PR adds the ability to support defining a default provider, directly inside the
@inject
decorator. Passing both adefaultProvider
andisOptional:true
, will simply use thedefaultProvider
, makingisOptional: true
useless in this case. We could add a warning or error but I think that doing the right thing, which is to use the default Provider is the right call.Maybe the best thing we should do is use a conditional type like this, so devs can know that it's one or the other.
I think however we would need to increase the version of TypeScript to support this, which I can do in a future version.
This will resolve: #163