- 
                Notifications
    
You must be signed in to change notification settings  - Fork 176
 
          Supports passing a default provider in the inject decorator.
          #264
        
          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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| import {defineInjectionTokenMetadata} from "../reflection-helpers"; | ||
| import InjectionToken, {TokenDescriptor} from "../providers/injection-token"; | ||
| import {Provider} from "../providers"; | ||
| import {instance as globalContainer} from "../dependency-container"; | ||
| 
     | 
||
| /** | ||
| * Parameter decorator factory that allows for interface information to be stored in the constructor's metadata | ||
| 
        
          
        
         | 
    @@ -8,7 +10,10 @@ import InjectionToken, {TokenDescriptor} from "../providers/injection-token"; | |
| */ | ||
| function inject( | ||
| token: InjectionToken<any>, | ||
| options?: {isOptional?: boolean} | ||
| options?: { | ||
| isOptional?: boolean; | ||
| defaultProvider?: Provider<any>; | ||
| } | ||
| ): ( | ||
| target: any, | ||
| propertyKey: string | symbol | undefined, | ||
| 
        
          
        
         | 
    @@ -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. | ||
| globalContainer.register(token, options.defaultProvider); | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like  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 commentThe 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 commentThe 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  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 commentThe 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.  | 
||
| } | ||
| 
     | 
||
| return defineInjectionTokenMetadata(data); | ||
| } | ||
| 
     | 
||
| 
          
            
          
           | 
    ||
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.