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

refactor(core): extract instance wrapper merge logic #9915

Merged
merged 15 commits into from
Jul 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions packages/core/injector/helpers/provider-classifier.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import {
ClassProvider,
FactoryProvider,
Provider,
ValueProvider,
} from '@nestjs/common';
import { isUndefined } from '@nestjs/common/utils/shared.utils';

export function isClassProvider<T = any>(
provider: Provider,
): provider is ClassProvider<T> {
return Boolean((provider as ClassProvider<T>)?.useClass);
}

export function isValueProvider<T = any>(
provider: Provider,
): provider is ValueProvider<T> {
const providerValue = (provider as ValueProvider)?.useValue;
return !isUndefined(providerValue);
}

export function isFactoryProvider<T = any>(
provider: Provider,
): provider is FactoryProvider<T> {
return Boolean((provider as FactoryProvider).useFactory);
}
20 changes: 13 additions & 7 deletions packages/core/injector/instance-wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ import {
} from '@nestjs/common/utils/shared.utils';
import { iterate } from 'iterare';
import { STATIC_CONTEXT } from './constants';
import {
isClassProvider,
isFactoryProvider,
isValueProvider,
} from './helpers/provider-classifier';
import { InstanceToken, Module } from './module';

export const INSTANCE_METADATA_SYMBOL = Symbol.for('instance_metadata:cache');
Expand Down Expand Up @@ -384,22 +389,23 @@ 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to set the scope to DEFAULT in this case? @kamilmysliwiec

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thiagomini static values are always singletons

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the reply!


this.setInstanceByContextId(STATIC_CONTEXT, {
instance: (provider as ValueProvider).useValue,
instance: provider.useValue,
isResolved: true,
isPending: false,
});
} else if ((provider as ClassProvider).useClass) {
} else if (isClassProvider(provider)) {
this.inject = null;
this.metatype = (provider as ClassProvider).useClass;
} else if ((provider as FactoryProvider).useFactory) {
this.metatype = (provider as FactoryProvider).useFactory;
this.inject = (provider as FactoryProvider).inject || [];
this.metatype = provider.useClass;
} else if (isFactoryProvider(provider)) {
this.metatype = provider.useFactory;
this.inject = provider.inject || [];
}
}

Expand Down
132 changes: 132 additions & 0 deletions packages/core/test/injector/helpers/provider-classifier.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
import { ClassProvider, FactoryProvider, ValueProvider } from '@nestjs/common';
import { expect } from 'chai';
import {
isClassProvider,
isFactoryProvider,
isValueProvider,
} from '../../../injector/helpers/provider-classifier';

describe('provider classifier', () => {
describe('isClassProvider', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 FactoryProvider

The 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?

it('should return true if useClass is present', () => {
const classProvider: ClassProvider = {
useClass: class TestClass {},
provide: 'token',
};

expect(isClassProvider(classProvider)).to.be.true;
});

it('should return false if useClass is undefined', () => {
const classProvider: ClassProvider = {
useClass: undefined,
provide: 'token',
};

expect(isClassProvider(classProvider)).to.be.false;
});

it('should return false if useClass is not present', () => {
const classProvider = {
provide: 'token',
};

expect(isClassProvider(classProvider as ClassProvider)).to.be.false;
});

it('should return false if provider is undefined', () => {
const classProvider = undefined;

expect(isClassProvider(classProvider as ClassProvider)).to.be.false;
});
});

describe('isValueProvider', () => {
it('should return true if useValue is not undefined', () => {
const valueProvider: ValueProvider = {
useValue: 'value',
provide: 'token',
};

expect(isValueProvider(valueProvider)).to.be.true;
});

it('should return true if useValue is "false"', () => {
const valueProvider: ValueProvider = {
useValue: false,
provide: 'token',
};

expect(isValueProvider(valueProvider)).to.be.true;
});

it('should return true if useValue is "null"', () => {
const valueProvider: ValueProvider = {
useValue: null,
provide: 'token',
};

expect(isValueProvider(valueProvider)).to.be.true;
});

it('should return true if useValue is an empty string', () => {
const valueProvider: ValueProvider = {
useValue: null,
provide: '',
};

expect(isValueProvider(valueProvider)).to.be.true;
});

it('should return false if useValue is undefined', () => {
const valueProvider: ValueProvider = {
useValue: undefined,
provide: 'token',
};

expect(isValueProvider(valueProvider)).to.be.false;
});

it('should return false if useValue is not present', () => {
const valueProvider = {
provide: 'token',
};

expect(isValueProvider(valueProvider as ValueProvider)).to.be.false;
});

it('should return false if provider is undefined', () => {
const valueProvider = undefined;

expect(isValueProvider(valueProvider as ValueProvider)).to.be.false;
});
});

describe('isFactoryProvider', () => {
it('should return true if useFactory is present', () => {
const factoryProvider: FactoryProvider = {
provide: 'token',
useFactory: () => {},
};

expect(isFactoryProvider(factoryProvider)).to.be.true;
});

it('should return false if useFactory is not present', () => {
const factoryProvider = {
provide: 'token',
};

expect(isFactoryProvider(factoryProvider as FactoryProvider)).to.be.false;
});

it('should return false if useFactory is undefined', () => {
const factoryProvider: FactoryProvider = {
provide: 'token',
useFactory: undefined,
};

expect(isFactoryProvider(factoryProvider as FactoryProvider)).to.be.false;
});
});
});
64 changes: 64 additions & 0 deletions packages/core/test/injector/instance-wrapper.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,4 +434,68 @@ describe('InstanceWrapper', () => {
});
});
});

describe('mergeWith', () => {
describe('when provider is a ValueProvider', () => {
it('should provide the given value in the STATIC_CONTEXT', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate a careful review of these tests to guarantee I tested the correct outcome for each case

const wrapper = new InstanceWrapper();
wrapper.mergeWith({
useValue: 'value',
provide: 'token',
});

expect(
wrapper.getInstanceByContextId(STATIC_CONTEXT).instance,
).to.be.equal('value');
});
});

describe('when provider is a ClassProvider', () => {
it('should alter the instance wrapper metatype with the given class', () => {
const wrapper = new InstanceWrapper();

wrapper.mergeWith({
useClass: TestClass,
provide: 'token',
});

expect(wrapper.metatype).to.be.eql(TestClass);
});
});

describe('when provider is a FactoryProvider', () => {
describe('and it has injected dependencies', () => {
it('should alter the instance wrapper metatype and inject attributes with the given values', () => {
const wrapper = new InstanceWrapper();

const factory = (_dependency1: any, _dependency2: any) => {};
const injectedDependencies = ['dependency1', 'dependency2'];

wrapper.mergeWith({
provide: 'token',
useFactory: factory,
inject: injectedDependencies,
});

expect(wrapper.metatype).to.be.eql(factory);
expect(wrapper.inject).to.be.eq(injectedDependencies);
});
});

describe('and it has no injected dependencies', () => {
it('should alter the instance wrapper metatype with the given values', () => {
const wrapper = new InstanceWrapper();
const factory = (_dependency1: any, _dependency2: any) => {};

wrapper.mergeWith({
provide: 'token',
useFactory: factory,
});

expect(wrapper.metatype).to.be.eql(factory);
expect(wrapper.inject).to.be.eql([]);
});
});
});
});
});