Add injectNames to use DI in react2angular #49
Conversation
There was a problem hiding this comment.
Hey @fand! This is a fantastic idea! I was using this lib with ngimport, but for those users that aren't this is really helpful!
Please address the few comments I left, then I'm happy to merge this. After merge I'll update the docs, and will work on making the DI typesafe (ie. if I pass $http to a React component, $http should be well-typed and not an any).
package.json
Outdated
| @@ -57,7 +57,7 @@ | |||
| "@types/angular": ">=1.5", | |||
| "@types/lodash.frompairs": "^4.0.3", | |||
| "@types/react": "^16.0.0", | |||
| "@types/react-dom": "^16.0.0", | |||
| "@types/react-dom": "16.0.1", | |||
There was a problem hiding this comment.
Nope. When I install @types/[email protected] or later version it breaks the build, so we have to specify 16.0.1 😇
index.tsx
Outdated
| ): IComponentOptions { | ||
| const names = bindingNames | ||
| || (Class.propTypes && Object.keys(Class.propTypes)) | ||
| || [] | ||
|
|
||
| return { | ||
| bindings: fromPairs(names.map(_ => [_, '<'])), | ||
| controller: ['$element', class extends NgComponent<Props> { | ||
| constructor(private $element: IAugmentedJQuery) { | ||
| controller: ['$element', ...(injectNames as any), class extends NgComponent<Props> { |
There was a problem hiding this comment.
Can you remove the as any here?
index.tsx
Outdated
| } | ||
| render() { | ||
| // TODO: rm any when https://github.com/Microsoft/TypeScript/pull/13288 is merged | ||
| render(<Class {...(this.props as any)} />, this.$element[0]) | ||
| render(<Class {...(this.props as any)} {...this.injectedProps} />, this.$element[0]) |
There was a problem hiding this comment.
Consider reversing the order of {...this.injectedProps} and {...(this.props as any)}, so props take priority.
There was a problem hiding this comment.
Uh, okay. Fixed the order and added some tests!
| it('should use the injectNames for DI', () => { | ||
| const defaultDi = (react2angular(TestThree).controller as any).slice(0, -1) | ||
| const injectedDi = (react2angular(TestThree, null, ['foo', 'bar']).controller as any).slice(0, -1) | ||
| expect(injectedDi).toEqual(defaultDi.concat(['foo', 'bar'])) |
There was a problem hiding this comment.
Instead of testing arity, please add broader tests. For example, you might consider injecting $http, spying on $http.get, and then calling it from the controller.
Also please add a test for a custom (not built-in) service: register it with angular.service, inject it, spy on it, and similarly make sure it gets called.
fand
changed the title
|
@fand I missed your updates earlier - is this PR ready to go? |
|
This is cool feature for those who are not using |
|
See above comment - if @fand believes this is ready to go, I'll go ahead and merge it. If they don't respond in a day or two I'll make the judgement myself. |
|
@bcherny |
|
Looks great - thanks for the contribution @fand! |
Currently react2angular cannot use the DI system of AngularJS.
In this PR I added
injectNamesto react2angular to specify injectables to converted components.When I do this:
then injected items will be passed to the props like this:
This PR is based on #48 in order to run tests locally.
Commits before 1a3ae9f is not necessary for this PR.