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

Open
wants to merge 1 commit into
base: context-watcher
from

Conversation

Projects
None yet
4 participants
@raymondfeng
Copy link
Member

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.

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 Jan 15, 2019

@raymondfeng raymondfeng changed the base branch from master to context-watcher Jan 15, 2019

@jannyHou

This comment has been minimized.

Copy link
Contributor

jannyHou commented Jan 15, 2019

@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?

@raymondfeng raymondfeng force-pushed the greeter-extension branch from fc53201 to 53ae291 Jan 15, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch from 167668d to a0f6b74 Jan 15, 2019

@bajtos

This comment has been minimized.

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

This comment has been minimized.

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 Jan 15, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng 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?

Yes. The PR is against context-watcher branch.

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng 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?

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 context-watcher branch from 765f316 to b25d990 Jan 15, 2019

@raymondfeng raymondfeng force-pushed the greeter-extension branch from a4c9ffb to 25ad6d9 Jan 15, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch from b25d990 to b9518c1 Jan 17, 2019

@raymondfeng raymondfeng force-pushed the greeter-extension branch from 44742f9 to b75bc99 Jan 17, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch from 01866ed to 878e523 Jan 22, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch from 878e523 to d738173 Jan 23, 2019

@raymondfeng raymondfeng force-pushed the greeter-extension branch 2 times, most recently from accacaf to 112dea3 Jan 23, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch from 3ea2f02 to 180ccb9 Jan 24, 2019

@raymondfeng raymondfeng force-pushed the greeter-extension branch from 112dea3 to 8c8585e Jan 24, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch 2 times, most recently from e5d289c to f81045f Jan 24, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch from e7668e4 to 34be9a3 Feb 7, 2019

@raymondfeng raymondfeng force-pushed the greeter-extension branch from 6dfd663 to 201d4ea Feb 7, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch 2 times, most recently from a49b82b to 80c9513 Feb 9, 2019

@raymondfeng raymondfeng force-pushed the greeter-extension branch from 201d4ea to 039d14b Feb 10, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch 2 times, most recently from df24e83 to 314ff59 Feb 11, 2019

@raymondfeng raymondfeng force-pushed the greeter-extension branch from 039d14b to 71cb1c4 Feb 12, 2019

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Feb 12, 2019

@raymondfeng it makes me wonder. Instead of building a made-up example, would it be better to update our @loopback/authentication or @loopback/boot packages to use these new features first? That way we can test the new extensibility features in a more realistic scenarios and thus have a higher chance of discovering missing pieces or rough edges.

I agree it does make sense to have an example showing extension points in isolation, making it easier to understand for readers. I am just not sure if now is the right time to work on that example, before we reworked our existing packages to use these new mechanisms?

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Feb 12, 2019

My intention is to make the business logic very simple to avoid any noises. The example represents the minimal requirements for any module that needs to implement extension point/extension pattern with LB4 core:

  1. The extension point defines an interface for extensions to implement
  2. Extensions can be added independent of the extension point
  3. The extension point need to access all registered extensions
  4. Extensions can come and go
  5. The extension point and extensions should be able to accept configuration

@raymondfeng raymondfeng force-pushed the context-watcher branch 2 times, most recently from c7a2e0c to 355f4f6 Feb 12, 2019

@raymondfeng raymondfeng force-pushed the greeter-extension branch from 71cb1c4 to 1a47923 Feb 12, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch 2 times, most recently from 9e2e739 to e5340fd Feb 12, 2019

@raymondfeng raymondfeng changed the title [RFC] Greeter extension to illustrate extension point/extension pattern feat: add greeter extension to illustrate extension point/extension pattern Feb 12, 2019

@raymondfeng raymondfeng force-pushed the greeter-extension branch from 1a47923 to 5047ec1 Feb 12, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch from e5340fd to bf5cf26 Feb 14, 2019

@bajtos
Copy link
Member

bajtos left a comment

There are too many unrelated changes included in the pull request, it makes the patch difficult to review. I'll wait with further review until related Context improvements are landed and this PR is rebased on top of them.

/**
* An extension point for greeters that can greet in different languages
*/
export class GreeterExtensionPoint {

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

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.

If we want to better show the implementation details of an extension point, then I am proposing a slightly different design:

  • Greeter interface remains without changes, this part is already designed wells.
  • GreeterExtensionPoint that accepts the requested language and returns Greeter instance matching the language.
  • GreeterService that provides greet method and uses GreeterExtensionPoint to obtain the greeter instance to use.
export class GreeterExtensionPoint {
  constructor(
    @inject.getter(bindingTagFilter({extensionPoint: 'greeter'}))
    private greeters: Getter<Greeter[]>,
  ) {}

  findByLanguage(language: string): Greeter | undefined {
    const greeters = await this.greeters();
    return greeters.find(g => g.language === language);
  }
}

class GreeterService {
  constructor(
    @inject('extensions.greeter')
    private greeterRegistry: GreeterExtensionPoint
    // ...
  ) {}

  async greet(language: string, name: string): Promise<string> {
    const greeter = await greeterRegistry.findByLanguage(language);
    const greeting = greeter ? greeter.greet(name) : `Hello, ${name}`;
    // etc.
  }
}

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

I feel it would be even better if we could find a way how to refactor GreeterExtensionPoint into something more generic that can be configured via @inject decorator. Below is the first version that comes to my mind. It's not as pretty as I would like it to be, we may need to look for a different approach.

type GreeterLocator = (lang: string) => Greeter;

class GreeterService {
  constructor(
    @inject.extensionLocator(
       'extensions.greeter', 
       (lang: string) => (ext: Greeter) => ext.language === lang)
    private findGreeterByLanguage: GreeterLocator) {}

   async greet(language: string, name: string): Promise<string> {
    const greeter = await findGreeterByLanguage(language);
    // etc.
  }
}

// resolver for extensionLocator
// the next two values will be obtained from injection metadata
const extensionPointName = // 'extensions.greeter';
const filterFactory = // (lang: string) => (ext: Greeter) => ext.language === lang);
const extensions = await new ContextView(ctx, filterByExtension(extensionPointName)).values();
return (...args) => {
  const filter = filterFactory(...args);
  return extensions.find(filter);
};

Maybe the complexity is not worth the benefits? IDK.

Anyways, this is probably out of scope of this pull request.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 15, 2019

Author Member

I was experimenting generalization of extension points into a base class at https://github.com/strongloop/loopback-next/tree/extension-point. My current assessment is that the benefit is not significant enough.

key: 'greeter-extension-point',
}),
createBindingFromClass(EnglishGreeter, {namespace: 'greeters'}),
createBindingFromClass(ChineseGreeter, {namespace: 'greeters'}),

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

Can we use asGreeter template please?

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

I mean why the built-in greeters are created with {namespace: 'greeters'} but without applying the changes made by asGreeter and why asGreeter does not set any namespace?

*
* @param extensionPoint Name/id of the extension point
*/
export function extensions(extensionPoint: string) {

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

Perhaps injectExtensions to be promoted to @inject.extensions may be a better name?

This comment has been minimized.

@raymondfeng

raymondfeng Feb 15, 2019

Author Member

Yes. Once we are settled on the final approach.

@raymondfeng raymondfeng force-pushed the context-watcher branch 2 times, most recently from b5ae9a8 to 2dac020 Feb 15, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Feb 15, 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.

@raymondfeng raymondfeng force-pushed the greeter-extension branch 2 times, most recently from 14fccf0 to 3ff92dd Feb 15, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch from 2dac020 to b71eedb Feb 20, 2019

feat: add greeter-extension example
The example illustrates how to implement extension point/extenion
pattern in LoopBack 4 to provide great extensibility.

@raymondfeng raymondfeng force-pushed the greeter-extension branch from 3ff92dd to ab70385 Feb 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment