refactor(core): extract instance wrapper merge logic #9915
Conversation
ensure isClassProvider works as expected
ensure isValueProvider returns true when useValue is defined
ensure useValue returns false for undefined values
ensure isValueProvider returns false when useValue is not present
ensure isValueProvider returns false when given provider is undefined
ensure isFactoryProvider returns true when useFactory is present
ensure isFactoryProvider returns false when useFactory is not present
ensure isFactoryProvider returns false when useFactory is undefined
ensure isValueProvider returns true for falsy values
refactor instance-wrapper to use provider classifier methods
ensure mergeWith handles ValueProviders
ensure mergeWith handles ClassProviders
ensure mergeWith handles FactoryProviders with injected dependencies
ensure mergeWith handles FactoryProviders with no depencies
thiagomini
changed the title
thiagomini
changed the title
Pull Request Test Coverage Report for Build 6b68e907-67bc-4104-9eab-f1293def3c0e
💛 - Coveralls |
| @@ -384,7 +389,7 @@ export class InstanceWrapper<T = any> { | |||
| } | |||
|
|
|||
| public mergeWith(provider: Provider) { | |||
| if (!isUndefined((provider as ValueProvider).useValue)) { | |||
| if (isValueProvider(provider)) { | |||
| this.metatype = null; | |||
| this.inject = null; | |||
| this.scope = Scope.DEFAULT; | |||
There was a problem hiding this comment.
Why do we have to set the scope to DEFAULT in this case? @kamilmysliwiec
There was a problem hiding this comment.
@thiagomini static values are always singletons
There was a problem hiding this comment.
Makes sense, thanks for the reply!
| } from '../../../injector/helpers/provider-classifier'; | ||
|
|
||
| describe('provider classifier', () => { | ||
| describe('isClassProvider', () => { |
There was a problem hiding this comment.
For each of these utility functions, it may be useful to have some runtime checks to guarantee we're dealing with correct providers. For instance, in the case of a FactoryProvider that provides something like:
const wrongFactoryProvider = {
provide: 'token',
useFactory: {}
} as FactoryProviderThe object itself is not a real function. Should this really be recognized as a FactoryProvider in this case? Or we should rather throw an error informing the given value is not a real function?
|
|
||
| describe('mergeWith', () => { | ||
| describe('when provider is a ValueProvider', () => { | ||
| it('should provide the given value in the STATIC_CONTEXT', () => { |
There was a problem hiding this comment.
I appreciate a careful review of these tests to guarantee I tested the correct outcome for each case
| } from '@nestjs/common'; | ||
| import { isUndefined } from '@nestjs/common/utils/shared.utils'; | ||
|
|
||
| export function isClassProvider(provider: Provider): boolean { |
There was a problem hiding this comment.
I guess we could use ts guards here like
| export function isClassProvider(provider: Provider): boolean { | |
| export function isClassProvider<T = any>(provider: Provider): provider is ClassProvider<T> { |
then we'll be able to remove those type assertions on statements below isClassProvider calls, I guess
There was a problem hiding this comment.
Good suggestion Micael! Thanks 👍
change provider classifier functions to work as type guards
kamilmysliwiec
added
type: enhancement 🐺
scope: core
type: refactor
and removed
type: enhancement 🐺
labels
|
LGTM |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The
InstanceWrapper#mergeWithmethod was using some verifications to determine how it would merge with a given provider. I extracted this logic to another file to improve its readability and also because it could be potentially used by other classes.Moreover, I added some missing test cases for it.
What is the new behavior?
No new behavior, only refactor of existing one.
Does this PR introduce a breaking change?
Other information