feat(context): add support to sort bindings for `@inject.*` and `ContextView` #2848

raymondfeng · 2019-05-07T16:36:03Z

This PR introduces BindingSorter to help sort bindings discovered from contexts. Some extension points need to control the order of bound extensions. Such APIs can be used to sort life cycle observers
and global interceptors after the PR is landed.

It has two commits:

Add BindingComparator interface and a utility to sort by group tag.
Add support of bindingComparator for ContextView and @inject.*.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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

👉 Check out how to submit a PR 👈

jannyHou

👍 Mostly LGTM. I left a comment about the order of unknown group.

packages/context/src/__tests__/unit/binding-sorter.unit.ts

emonddr · 2019-05-08T13:48:55Z

@raymondfeng , tiny typo.

Some extension points need to control of order of bound extensions.

Some extension points need to control the order of bound extensions.

emonddr · 2019-05-08T13:51:32Z

@raymondfeng
Question: what is it's referring to?

Such APIs can be used to sort life cycle observers and global interceptors after it's landed.
If it's refers to the life cycle observers and global interceptors, we should use they . If it refers to something else, please clarify. Thanks.

emonddr · 2019-05-08T13:56:24Z

@raymondfeng

If you could please provide an example of when it helps that bindings are sorted. I thought bindings were stored in a hashmap and were resolved quickly. Is it more for a programmer that is writing a specific piece of logic that needs to pass through a list in a particular fashion?

emonddr

Looks good. Just have a few questions in the comments area.

raymondfeng · 2019-05-08T14:58:08Z

If you could please provide an example of when it helps that bindings are sorted. I thought bindings were stored in a hashmap and were resolved quickly. Is it more for a programmer that is writing a specific piece of logic that needs to pass through a list in a particular fashion?

Bindings are stored in a Map by keys. No sorting is involved when a binding is resolved by key. The binding sorter is only allowed for @inject.* or ContextView that takes a BindingFilter function to select multiple bindings, such as:

Registered life cycle observers
Request body parsers
Global interceptors

b-admike

Got one question, otherwise LGTM.

packages/context/src/__tests__/unit/binding-sorter.unit.ts

packages/context/src/inject.ts

bajtos

I like the idea of allowing @inject* consumers to specify ordering of the injected bindings 👍

The implementation looks good at design level.

However, I find the naming convention rather problematic. I feel the name sorter implies the function will perform the sort, while in reality it's a compareFunction passed to Array.prototype.sort. Let's find a better name please and update all places accordingly.

docs/site/Decorators_inject.md

packages/context/src/__tests__/unit/context-view.unit.ts

packages/context/src/binding-sorter.ts

packages/context/src/context-view.ts

packages/context/src/inject.ts

bajtos · 2019-05-09T08:11:56Z

@raymondfeng How do you envision integration & interaction of this new feature with Extension Point/Extension pattern? Can code receiving a list of extension bindings use this new feature out of the box?

raymondfeng · 2019-05-09T14:27:29Z

How do you envision integration & interaction of this new feature with Extension Point/Extension pattern? Can code receiving a list of extension bindings use this new feature out of the box?

Yes, I'll refactor some of the extension points to use this new feature in two flavors:

Use bindingComparator for @extensions
Use bindingComparator for ContextView or @inject.view

docs/site/Decorators_inject.md

packages/context/src/binding-sorter.ts

bajtos

Looks mostly good, I have few more minor comments to consider.

No further review is necessary as far as I am concerned, but please get at least one or two more approvals from other team members.

bajtos · 2019-05-13T06:42:03Z

packages/rest/src/body-parsers/body-parser.ts


  constructor(
-    @inject.tag(REQUEST_BODY_PARSER_TAG, {optional: true})
+    @inject(filterByTag(REQUEST_BODY_PARSER_TAG), {optional: true})


What is the reasoning for this change? Is it just a stylistic refactoring, or does it introduce any changes in the externally-observed behavior, e.g. the order in which the parsers are invoked?

I am ok with the change, but would like to better understand the consequences.

It's stylistic. IIRC, we would like to deprecate @inject.tag in the future and use @inject with a filter function instead.

bajtos · 2019-05-13T06:43:44Z

packages/context/src/context-view.ts

+ * @param bindingComparatorOrSession A function to sort matched bindings or
+ * resolution session if the comparator is not needed
+ * @param session Resolution session if the comparator is provided
+ */


I think the actual implementation should not have any tsdoc comments. We have already described the two flavors recognized by the compiler when validating places calling this method. WDYT?

The comments are for maintainers only. I assume strong-docs or tsdocs only expose the method signatures (not implementation).

packages/context/src/inject.ts

@Inject

- add ability to sort bindings by tag values - allow binding sorter on ContextView and @Inject.*

raymondfeng requested a review from bajtos as a code owner May 7, 2019 16:36

raymondfeng mentioned this pull request May 7, 2019

feat(context): add support for method interceptors #2687

Merged

7 tasks

raymondfeng force-pushed the binding-sorter branch from 0572f73 to f9f7515 Compare May 7, 2019 16:38

jannyHou reviewed May 7, 2019

View reviewed changes

packages/context/src/__tests__/unit/binding-sorter.unit.ts Show resolved Hide resolved

raymondfeng force-pushed the binding-sorter branch from 2aaa0b7 to b024f5f Compare May 7, 2019 18:03

raymondfeng requested a review from jannyHou May 7, 2019 18:05

raymondfeng added Extensions feature IoC/Context @loopback/context: Dependency Injection, Inversion of Control labels May 7, 2019

raymondfeng force-pushed the binding-sorter branch from b024f5f to 9192a21 Compare May 7, 2019 19:54

emonddr approved these changes May 8, 2019

View reviewed changes

b-admike approved these changes May 8, 2019

View reviewed changes

packages/context/src/__tests__/unit/binding-sorter.unit.ts Outdated Show resolved Hide resolved

raymondfeng force-pushed the binding-sorter branch from 9192a21 to 05a770f Compare May 8, 2019 16:26

b-admike reviewed May 8, 2019

View reviewed changes

packages/context/src/inject.ts Show resolved Hide resolved

bajtos suggested changes May 9, 2019

View reviewed changes

raymondfeng force-pushed the binding-sorter branch from 05a770f to 5be908f Compare May 9, 2019 14:17

raymondfeng changed the title ~~feat(context): add binding sorter~~ feat(context): add support to sort bindings for @inject.* and ContextView May 9, 2019

raymondfeng force-pushed the binding-sorter branch 3 times, most recently from c25308f to 89ab031 Compare May 9, 2019 14:55

raymondfeng requested a review from bajtos May 9, 2019 14:56

raymondfeng force-pushed the binding-sorter branch 3 times, most recently from d7d5657 to add0552 Compare May 9, 2019 19:07

raymondfeng force-pushed the binding-sorter branch from 9d1ac7d to 0b0a9b9 Compare May 9, 2019 20:52

bajtos reviewed May 10, 2019

View reviewed changes

docs/site/Decorators_inject.md Show resolved Hide resolved

packages/context/src/binding-sorter.ts Outdated Show resolved Hide resolved

packages/context/src/binding-sorter.ts Outdated Show resolved Hide resolved

raymondfeng force-pushed the binding-sorter branch from 0b0a9b9 to 5b0407f Compare May 10, 2019 17:07

bajtos approved these changes May 13, 2019

View reviewed changes

raymondfeng added 2 commits May 13, 2019 07:16

feat(context): add binding comparator to sort bindings

bb35542

- add ability to sort bindings by tag values - allow binding sorter on ContextView and @Inject.*

chore: refactor code to use binding sorter

428325f

raymondfeng force-pushed the binding-sorter branch from 5b0407f to 428325f Compare May 13, 2019 14:17

raymondfeng merged commit db1c004 into master May 13, 2019

raymondfeng deleted the binding-sorter branch May 13, 2019 14:52

feat(context): add support to sort bindings for @inject.* and ContextView #2848

feat(context): add support to sort bindings for @inject.* and ContextView #2848

Uh oh!

Conversation

raymondfeng commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emonddr commented May 8, 2019

Uh oh!

emonddr commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emonddr commented May 8, 2019

Uh oh!

emonddr left a comment

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented May 8, 2019

Uh oh!

b-admike left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bajtos commented May 9, 2019

Uh oh!

raymondfeng commented May 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos May 13, 2019

Choose a reason for hiding this comment

Uh oh!

raymondfeng May 13, 2019

Choose a reason for hiding this comment

Uh oh!

bajtos May 13, 2019

Choose a reason for hiding this comment

Uh oh!

raymondfeng May 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

feat(context): add support to sort bindings for `@inject.*` and `ContextView` #2848

feat(context): add support to sort bindings for `@inject.*` and `ContextView` #2848

raymondfeng commented May 7, 2019 •

edited

Loading

emonddr commented May 8, 2019 •

edited

Loading