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.

Sign up for GitHub

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

Jump to bottom

Add support for property dependency injection #292

Merged
merged 1 commit into from May 25, 2017
Merged

Add support for property dependency injection #292

merged 1 commit into from May 25, 2017

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng requested a review from bajtos May 22, 2017 23:23
bajtos
bajtos reviewed May 23, 2017
View reviewed changes
packages/context/test/unit/inject.test.ts Outdated
@@ -22,7 +22,8 @@ describe('function argument injection', () => {
}

const meta = describeInjectedArguments(TestClass);
expect(meta).to.deepEqual(['foo']);
expect(meta.length).to.eql(1);
expect(meta[0].bindingKey).to.deepEqual('foo');
Copy link
Member

Choose a reason for hiding this comment

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

expect(meta.map(m => m.bindingKey)).to.deepEqual(['foo']);

This way the assertion error message can include extra keys when the meta has more than one entry.

bajtos
bajtos reviewed May 23, 2017
View reviewed changes
packages/context/test/unit/resolver.test.ts Outdated
}
}

let t = instantiateClass<TestClass>(TestClass, ctx) as TestClass;
Copy link
Member

Choose a reason for hiding this comment

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

I think the TypeScript compiler should be able to infer the specialisation of instantiateClass from the first argument:

let t = instantiateClass(TestClass, ctx) as TestClass;

Could you please double check?

If I am right, then the same simplification should be applied to most of the other tests below too.

Copy link
Member

Choose a reason for hiding this comment

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

Also we should prefer const over let, I am surprised tslint did not complain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note the return type of instantiateClass is T | Promise<T>. We have to cast it.

bajtos
bajtos suggested changes May 23, 2017
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Looks mostly good, I left few comments to address.

packages/context/test/unit/resolver.test.ts Outdated

let t = instantiateClass<TestClass>(TestClass, ctx) as TestClass;
expect(t.foo).to.eql('FOO');

Copy link
Member

Choose a reason for hiding this comment

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

Extra empty line, please remove.

Same comment applies to other test cases in this file too.

@bajtos
Copy link
Member

bajtos commented May 23, 2017

Also we should prefer const over let, I am surprised tslint did not complain.

Haha, tslint did catch that, see the output of Travis CI for an example (link). Please fix your code to passnpm test ;-)

@raymondfeng
Copy link
Contributor Author

@bajtos Comments addressed. PTAL.

@raymondfeng raymondfeng force-pushed the feature/inject-properties branch 2 times, most recently from 12d0f0f to 245311d Compare May 23, 2017 21:27
bajtos
bajtos suggested changes May 24, 2017
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Almost there :)

I found few inconsistencies in the code, PTAL 👇

packages/context/src/inject.ts Outdated
export function inject(bindingKey: string, metadata?: Object) {
// tslint:disable-next-line:no-any
return function markArgumentAsInjected(target: any, propertyKey?: string | symbol,
propertyDescritorOrParameterIndex?: TypedPropertyDescriptor<BoundValue> | number) {
Copy link
Member

Choose a reason for hiding this comment

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

Typo?

- propertyDescritorOrParameterIndex
+ propertyDescriptorOrParameterIndex

packages/context/src/resolver.ts
} else {
if (isPromise(inst)) {
// Inject the properties asynchrounously
return inst.then(obj => Object.assign(obj, propertiesOrPromise));
Copy link
Member

Choose a reason for hiding this comment

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

This is inconsistent with L39. Why is return obj needed on L39 but not needed here?

Similarly for L43, why is return inst needed?

packages/context/test/unit/resolver.test.ts Outdated
}
}

const t = await instantiateClass(TestClass, ctx) as TestClass;
Copy link
Member

Choose a reason for hiding this comment

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

On L123 below, you have the following code:

const t = await instantiateClass(TestClass, ctx);

Can we use the same approach here and avoid the extra as TestClass cast?

Please review other tests calling await instantiateClass too and make the code consistent.

@raymondfeng
Copy link
Contributor Author

@bajtos Fixed. PTAL.

bajtos
bajtos approved these changes May 25, 2017
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Please rebase the patch on top of the current master. Among other things, it will enable Coveralls to verify that test coverage of the code has not decreased.

@raymondfeng raymondfeng merged commit 8ae39b0 into master May 25, 2017
@raymondfeng raymondfeng deleted the feature/inject-properties branch May 25, 2017 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bajtos bajtos bajtos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@raymondfeng @bajtos