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

[WIP] feat(context): add @injectable to decorate bindable classes #992

Closed
wants to merge 1 commit into from
Closed

[WIP] feat(context): add @injectable to decorate bindable classes #992

wants to merge 1 commit into from

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Feb 13, 2018 •
edited
Loading

Implements #958

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Related API Documentation was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch 2 times, most recently from 1c8cf6d to 3d7e1bf Compare February 26, 2018 17:46
bajtos
bajtos reviewed Mar 1, 2018
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.

Please add acceptance tests showing how do you envision the integration of this feature with the application and the bootstrapper. How are we going to discover all types/classes decorated with @injectable?

packages/context/src/injectable.ts Outdated
* - server
* - class (default)
*/
type?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Please capture the valid values in the type definition.

type?: 'controller' | 'repository' | 'component' | // etc.

@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch 2 times, most recently from c4fcc09 to 03fd769 Compare March 2, 2018 21:31
@raymondfeng raymondfeng mentioned this pull request Mar 7, 2018
Merged
6 tasks
@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch 2 times, most recently from 96237b4 to 0382152 Compare March 7, 2018 22:04
@raymondfeng raymondfeng mentioned this pull request Mar 22, 2018
Closed
4 tasks
@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch from 0382152 to c470bfa Compare March 30, 2018 22:30
@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch 3 times, most recently from 979a428 to c8cdd80 Compare April 11, 2018 16:11
@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch from c8cdd80 to d1f5b59 Compare June 5, 2018 22:21
@bajtos bajtos added post-GA and removed LB4 GA labels Jun 28, 2018
@dhmlau dhmlau removed the non-DP3 label Aug 23, 2018
@raymondfeng raymondfeng mentioned this pull request Aug 30, 2018
Merged
7 tasks
@bajtos
Copy link
Member

bajtos commented Oct 26, 2018

There have been no updates in this pull request for many months, I am closing it for now. @raymondfeng feel free to reopen if you get time to continue this work. (Considering how outdated these changes are, it may be better to start from scratch.)

@bajtos bajtos closed this Oct 26, 2018
@raymondfeng raymondfeng mentioned this pull request Dec 6, 2018
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bajtos bajtos bajtos left review comments

@kjdelisle kjdelisle Awaiting requested review from kjdelisle

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

Successfully merging this pull request may close these issues.
3 participants
@raymondfeng @bajtos @dhmlau