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

[raymondfeng]

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

[jannyHou]

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

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]

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]

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]

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.

[bajtos]

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.

[bajtos]

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.
  }
}

[bajtos]

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.

[raymondfeng]

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.

[bajtos]

Can we use asGreeter template please?

[bajtos]

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?

[bajtos]

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

[raymondfeng]

Yes. Once we are settled on the final approach.

[raymondfeng]

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.

[bajtos]

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

• is a bad practice violating SOLID principles, in particular Single responsibility principle,
• and makes the example unnecessarily difficult to understand for new readers.

[raymondfeng]

@bajtos PTAL

[bajtos]

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.

[nabdelgadir]

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?

[jannyHou]

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

[emonddr]

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.

[jannyHou]

Thanks for clarifying all the questions! LGTM.

[bajtos]

👏