Skip to content

feat(core): allow an extension to be used by multiple extension points #4816

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
Merged
raymondfeng merged 3 commits into from
Mar 9, 2020
Merged

feat(core): allow an extension to be used by multiple extension points #4816

raymondfeng merged 3 commits into from
Mar 9, 2020

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Mar 8, 2020 •
edited
Loading

The PR is to address the need by #4693 (comment).

  1. Introduce TagValueMatcher function type for more flexible tag value matching used by filterByTag
  2. Use an array inclusion based TagValueMatcher to discover extensions tagged for multiple extension points.

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 👈

@raymondfeng raymondfeng requested a review from deepakrkris as a code owner March 8, 2020 01:25
@raymondfeng raymondfeng mentioned this pull request Mar 8, 2020
Merged
7 tasks
dougal83
dougal83 approved these changes Mar 8, 2020
View reviewed changes
Copy link
Contributor

@dougal83 dougal83 left a comment

Choose a reason for hiding this comment

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

Awesome 👍 Thank you!

@raymondfeng raymondfeng force-pushed the multiple-extension-points branch from c457e8d to 79eda47 Compare March 8, 2020 01:54
@raymondfeng raymondfeng force-pushed the multiple-extension-points branch from 79eda47 to e93f23d Compare March 8, 2020 01:55
@raymondfeng raymondfeng force-pushed the multiple-extension-points branch from e93f23d to cbaddf2 Compare March 8, 2020 02:44
dougal83
dougal83 approved these changes Mar 8, 2020
View reviewed changes
Copy link
Contributor

@dougal83 dougal83 left a comment

Choose a reason for hiding this comment

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

Confirmed to enable #4693 to contribute via multiple extension points.

dougal83 added a commit to dougal83/loopback-next that referenced this pull request Mar 8, 2020
@dougal83
refactor: update & refine to non breaking
7b9d383 
use loopbackio#4816, and alter to non breaking by eaving it to user to use binding decorators in app.

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
@raymondfeng raymondfeng force-pushed the multiple-extension-points branch from cbaddf2 to 0275dc5 Compare March 8, 2020 20:40
@raymondfeng
Copy link
Contributor Author

raymondfeng commented Mar 8, 2020

@dougal83 Thank you for the review. I further improved the PR to make filterByTag even more flexible. PTAL.

@raymondfeng raymondfeng requested a review from dougal83 March 8, 2020 20:45
bajtos
bajtos suggested changes Mar 9, 2020
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.

Great improvements in developer experience ❤️

The first commit looks mostly good to me. I have reservations about the complexity of the proposed design in the second commit, let's discuss.

@bajtos
Copy link
Member

bajtos commented Mar 9, 2020

The first commit looks mostly good to me. I have reservations about the complexity of the proposed design in the second commit, let's discuss.

Hmm, it looks like GitHub is listing the commits in the opposite order.

By "first commit" I meant 0275dc5 (allow extensions to be used by multiple extension points), "second commit" meant dd6da12

@bajtos bajtos added Extensions feature IoC/Context @loopback/context: Dependency Injection, Inversion of Control labels Mar 9, 2020
emonddr
emonddr approved these changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

Nice improvement!

@raymondfeng raymondfeng force-pushed the multiple-extension-points branch from 0275dc5 to 2a4fe1b Compare March 9, 2020 15:45
@raymondfeng raymondfeng requested a review from bajtos March 9, 2020 15:46
bajtos
bajtos approved these changes Mar 9, 2020
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.

Sounds good. I now use a similar syntax as https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach to pass in the following params:

(tagValue, tagName, tagMap)

Lovely! This way LB developers can re-use their existing knowledge about ES6 and perhaps even re-use small functional bits between forEach and TagValueMatcher places.

@bajtos
Copy link
Member

bajtos commented Mar 9, 2020 •
edited
Loading

  • Documentation in /docs/site was updated

I think these additions deserve documentation too. I think both docs for Context and for ExtensionPoint/Extension pattern should be updated.

(Feel free to open a follow-up PR for the docs.)

@raymondfeng raymondfeng force-pushed the multiple-extension-points branch from 2a4fe1b to 652e8e4 Compare March 9, 2020 18:06
@raymondfeng raymondfeng requested a review from agnes512 as a code owner March 9, 2020 18:06
@raymondfeng raymondfeng merged commit 3a77e32 into master Mar 9, 2020
@raymondfeng raymondfeng deleted the multiple-extension-points branch March 9, 2020 18:45
dougal83 added a commit to dougal83/loopback-next that referenced this pull request Mar 9, 2020
@dougal83
refactor: update & refine to non breaking
69ae770 
use loopbackio#4816, and alter to non breaking by eaving it to user to use binding decorators in app.

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deepakrkris deepakrkris Awaiting requested review from deepakrkris

@jannyHou jannyHou Awaiting requested review from jannyHou

@agnes512 agnes512 Awaiting requested review from agnes512

@hacksparrow hacksparrow Awaiting requested review from hacksparrow

@nabdelgadir nabdelgadir Awaiting requested review from nabdelgadir

3 more reviewers

@bajtos bajtos bajtos approved these changes

@dougal83 dougal83 dougal83 approved these changes

@emonddr emonddr emonddr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Extensions feature IoC/Context @loopback/context: Dependency Injection, Inversion of Control

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@raymondfeng @bajtos @dougal83 @emonddr