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.

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

feat: add greeter extension to illustrate extension point/extension pattern #2249

Merged
merged 2 commits into from
Mar 26, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Jan 15, 2019

The PR adds an example project to demonstrate how to implement extensibility using the extension point/extension pattern on top of LoopBack 4's IoC and DI foundation.

See https://github.com/strongloop/loopback-next/blob/greeter-extension/examples/greeter-extension/README.md.

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 requested a review from bajtos as a code owner January 15, 2019 00:22
@raymondfeng raymondfeng changed the base branch from master to context-watcher January 15, 2019 00:22
@jannyHou
Copy link
Contributor

@raymondfeng Good to have an example repository for extension. What's the difference between this example repo and the log extension in https://github.com/strongloop/loopback-next/tree/master/examples/log-extension?

@bajtos
Copy link
Member

bajtos commented Jan 15, 2019

What's the difference between this example repo and the log extension in https://github.com/strongloop/loopback-next/tree/master/examples/log-extension?

+1, I'd like to better understand the differences too.

@bajtos
Copy link
Member

bajtos commented Jan 15, 2019

Also IIUC, this pull request contains commits from other pull requests, for example #2122. Am I assuming correctly that you will rebase those commit away once the other PRs are landed?

@raymondfeng raymondfeng force-pushed the greeter-extension branch 2 times, most recently from 7c5e45a to a4c9ffb Compare January 15, 2019 19:24
@raymondfeng
Copy link
Contributor Author

Also IIUC, this pull request contains commits from other pull requests, for example #2122. Am I assuming correctly that you will rebase those commit away once the other PRs are landed?

Yes. The PR is against context-watcher branch.

@raymondfeng
Copy link
Contributor Author

What's the difference between this example repo and the log extension in https://github.com/strongloop/loopback-next/tree/master/examples/log-extension?

The log-extension focuses on demonstrating the common LB4 constructs for extensions, such as decorators, mixins, providers, and components.

The greeter-extension is dedicated to the extension point/extension pattern. It illustrates how to solve the common extensibility issue by implementing the pattern on top of LB4 constructs. See README for more details.

@raymondfeng raymondfeng changed the title [RFC] Greeter extension [RFC] Greeter extension to illustrate extension point/extension pattern Jan 15, 2019
@raymondfeng raymondfeng force-pushed the greeter-extension branch 2 times, most recently from accacaf to 112dea3 Compare January 24, 2019 20:00
@raymondfeng raymondfeng force-pushed the context-watcher branch 2 times, most recently from e5d289c to f81045f Compare January 24, 2019 20:37
@bajtos
Copy link
Member

bajtos commented Mar 21, 2019

I find this class name problematic. This class is not the extension point, it's a service providing multilingual greet function and tapping into the greeter extension point. The actual extension point is just a convention on how greeter implementations should be bound on the context and how the greeter service is looking them up.

Decoupling GreeterService from GreeterDispatcher is fine if you see the extension point is only responsible for dispatching requests to corresponding extensions. But it should also be possible to combine the responsibility of two into one class.

I am ok to allow users to combine the responsibility of two into one class, if they wish so.

However, I am arguing that such design

@raymondfeng
Copy link
Contributor Author

@bajtos PTAL

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.

The patch is starting to look very reasonable 👍

Please get at least one more (preferably 2-3) people from @strongloop/loopback-maintainers to review this proposal too.

examples/greeter-extension/README.md Outdated Show resolved Hide resolved
examples/greeter-extension/README.md Outdated Show resolved Hide resolved
examples/greeter-extension/README.md Outdated Show resolved Hide resolved
examples/greeter-extension/src/decorators.ts Outdated Show resolved Hide resolved
examples/greeter-extension/src/greeting-service.ts Outdated Show resolved Hide resolved
examples/greeter-extension/src/greeting-service.ts Outdated Show resolved Hide resolved
@bajtos bajtos requested a review from nabdelgadir March 22, 2019 13:41
@raymondfeng raymondfeng requested a review from bajtos March 22, 2019 14:48
Copy link
Contributor

@nabdelgadir nabdelgadir left a comment

Choose a reason for hiding this comment

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

I think as far as I understand, this looks good to me, besides some minor comments. Is this example also going to be added to the docs/cli?

examples/greeter-extension/LICENSE Outdated Show resolved Hide resolved
examples/greeter-extension/index.ts Outdated Show resolved Hide resolved
examples/greeter-extension/src/index.ts Outdated Show resolved Hide resolved
examples/greeter-extension/src/keys.ts Outdated Show resolved Hide resolved
examples/greeter-extension/README.md Outdated Show resolved Hide resolved
docs/site/MONOREPO.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

The signature of registering an extension point and extensions LGTM 👏
I left a few questions.

examples/greeter-extension/README.md Show resolved Hide resolved
examples/greeter-extension/src/types.ts Outdated Show resolved Hide resolved
examples/greeter-extension/src/decorators.ts Show resolved Hide resolved
examples/greeter-extension/src/types.ts Show resolved Hide resolved
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.

Great stuff!
The only thing to fix would be the test case 'supports options for the extension point' where there is usage of chalk to test an extension point option is in place. I have a comment which explains the problem.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying all the questions! LGTM.

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.

👏

examples/greeter-extension/README.md Outdated Show resolved Hide resolved
packages/context/src/context-view.ts Show resolved Hide resolved
The example illustrates how to implement extension point/extenion
pattern in LoopBack 4 to provide great extensibility.
@raymondfeng raymondfeng merged commit 9b09298 into master Mar 26, 2019
@raymondfeng raymondfeng deleted the greeter-extension branch March 26, 2019 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants