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

feat(context): add @bind to decorate classes with binding attributes #2129

Merged
merged 2 commits into from Dec 13, 2018
Merged

feat(context): add @bind to decorate classes with binding attributes #2129

merged 2 commits into from Dec 13, 2018

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Dec 6, 2018 •
edited

This PR is a reactivation of #992.

Checklist

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

@raymondfeng raymondfeng mentioned this pull request Dec 6, 2018
Merged
7 tasks
@raymondfeng raymondfeng changed the title feat(context): add @bind to decorate classes with more information feat(context): add @bind to decorate classes with binding attributes Dec 6, 2018
@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch 3 times, most recently from cf4e5a1 to bda048b Compare December 7, 2018 03:36
@bajtos
Copy link
Member

bajtos commented Dec 10, 2018

I read through the documentation, the proposal looks good at high level.

I'll try to find time to review it in more details later this week.

In the meantime, I'd like people from @strongloop/loopback-maintainers and especially @strongloop/sq-lb-apex to review these changes too.

@bajtos bajtos requested review from a team December 10, 2018 14:27
bajtos
bajtos suggested changes Dec 11, 2018
View reviewed changes
packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/index.ts Show resolved Hide resolved
packages/context/src/value-promise.ts Outdated Show resolved Hide resolved
packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch 5 times, most recently from 2a44d96 to 8d9ebcd Compare December 12, 2018 22:11
@raymondfeng
Copy link
Contributor Author

@bajtos PTAL

bajtos
bajtos approved these changes Dec 13, 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.

LGTM.

Please get at least one more approval from @strongloop/loopback-maintainers before landing.

b-admike
b-admike reviewed Dec 13, 2018
View reviewed changes
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

Some comments for tests in general (I'll review impl in another round). LGTM overall.

packages/context/test/unit/binding-inspector.test.ts Outdated Show resolved Hide resolved
packages/context/test/unit/binding-inspector.test.ts Outdated Show resolved Hide resolved
packages/context/test/unit/binding-inspector.test.ts Show resolved Hide resolved
packages/context/test/unit/binding-inspector.test.ts Show resolved Hide resolved
packages/context/test/unit/binding-inspector.test.ts Show resolved Hide resolved
packages/context/test/unit/binding-decorator.test.ts Show resolved Hide resolved
packages/context/test/acceptance/binding-decorator.feature.md
# Feature: @bind for classes representing various artifacts

- In order to automatically bind classes for various artifacts to a context
- As a developer
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we want the bullet points here? Or do we want to say something like As a developer, I want to decorate my classes to provide more metadata so that the bootstrapper can bind them to a context according to the metadata in order to automatically bind classes for various artifacts to a 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 follow other acceptance md files.

packages/context/test/acceptance/bind-decorator.acceptance.ts Outdated Show resolved Hide resolved
packages/context/test/acceptance/bind-decorator.acceptance.ts Outdated Show resolved Hide resolved
packages/context/test/acceptance/binding-decorator.feature.md Outdated Show resolved Hide resolved
@raymondfeng
Copy link
Contributor Author

@b-admike Thank you for the review comments. I have addressed most of them.

@raymondfeng raymondfeng merged commit 84c056e into master Dec 13, 2018
@raymondfeng raymondfeng deleted the add-injectable-decorator branch December 13, 2018 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b-admike b-admike b-admike left review comments

@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
3 participants
@raymondfeng @bajtos @b-admike