[WIP] feat(core): Add support for extension point/extension pattern on top of Context #657

raymondfeng · 2017-10-17T17:35:52Z

Extension point/extension is a pattern for extensibility.
This PR illustrates how we can support it using the Context apis
and naming conventions. It also serves as an invitation to discuss
if we should support such entities out of box.

Description

Related issues

connect to Extensions Architecture #543

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

virkt25

+1 for the overall implementation but have some questions I would like to know for clarification.

From the example what advantage does this provide? (A user can just bind multiple AuthStrategy classes and inject/call the one they want).
In this design it seems like a developer is responsible for choosing the final extension they wants to use ... can this work whereby all extensions mounted to an extension point are notified and triggered? (An example might be a NotificationManager -> send out email notif + slack notif + push notif)
Would you suggest servers be written as an extensionPoint?

virkt25 · 2017-10-18T03:21:18Z

packages/context/test/acceptance/extension-point.ts

+      .bind('authentication.strategies.local')
+      .toClass(LocalStrategy)
+      .inScope(BindingScope.SINGLETON)
+      .tag('authentication-strategy');


I'm not familiar with .tag() ... And reading this code I'm not sure where it's used. Can you explain it's usage?

There are a few ways to group a set of bindings as extensions for a given extension point.

Use naming convention such as authentication.strategies.*

Use tags (all bindings with tag 'authentication-strategy' will be treated as extensions for the authentication.manager extension point.

Use tags (all bindings with tag 'authentication-strategy' will be treated as extensions for the authentication.manager extension point.

I think it's a bad idea to have two possibly conflicting conventions for looking up extensions. For example, it allows you to bind authentication strategy as a controller too via ctx.bind('controllers.authentication').toClass(MyStrategy).tag('authentication-strategy'). I know this example is silly, but my concern is that users often do silly things and then we have to spend our precious time explaining them what they did wrong.

I personally prefer to get rid of Binding's .tag() method entirely, considering that we haven't used it anywhere so far, and we are already almost in Beta.

Alternatively, if you insist on supporting both naming convention and binding tags to look up extensions, then please capture this in acceptance tests, including how edge cases like my silly example above should be handled.

I'm not proposing to support both. Personally, I don't want to rely too much on the naming magic and prefer to use tag to explicitly group bound services. Tags can also be used as filters for @inject. If we visualize the Context, tags will be really useful too.

virkt25 · 2017-10-18T03:45:45Z

packages/context/test/acceptance/extension-point.feature.md

+
+# TBD
+
+- Should we go beyond naming conventions to make ExtensionPoint/Extension first class entity of the context?


I agree that we should provide interface/base classes OOTB. However, I think they should be part of a different package than @loopback/context, probably @loopback/core.

I'll move it into @loopback/core. This way, @loopback/context stays as the IoC container while core implements common patterns on top of Context, for example, application, extension point/extension, and component.

virkt25 · 2017-10-18T03:45:50Z

packages/context/test/acceptance/extension-point.ts

+    ctx.bind('authentication.manager').toClass(AuthenticationManager);
+    ctx.bind('authentication.config').to({
+      extensions: {
+        local: {


Once Kevin's PR lands for .options() or .withDependencies (whatever it ends up being) ... I think it would be better to leverage that then as it would make configuration easier by allowing this to be moved to the Class bindings below. (I think that will make for a better developer experience).

virkt25 · 2017-10-18T03:46:26Z

packages/context/test/acceptance/extension-point.feature.md

+# TBD
+
+- Should we go beyond naming conventions to make ExtensionPoint/Extension first class entity of the context?
+- Should we introduce decorators such as `@extensionPoint` and `@extension`?


What would @extension be used for? I assume @extensionPoint will be used for declaring the extensionPoint classes instead of extending ExtensionPoint.

@extension decorator can be used to denote the name of an extension point it contributes to.

virkt25 · 2017-10-18T04:25:17Z

packages/context/test/acceptance/extension-point.ts

+   * Get an instance of an extension by name
+   * @param name Name of the extension
+   */
+  async getExtension(name: string): Promise<EXT> {


Would it make sense for us to automatically return the extension if only one has been registered to an ExtensionPoint? --> or mark a default one to use? ... I think it makes for an better developer experience.

As in the example below -> A developer with only one AuthStrategy would find it tedious to have to specify local again and again ...

When an extension point manages one or more extensions, a few common delegating cases are involved:

Pick up one by the matching metadata. For example, an authentication action can use the authentication.manager extension point to delegate a corresponding strategy provider based on the decoration of the target controller method.

Invoke all extensions, for example, email/sms/push notifications.

Try extensions one by one until one of them can handle the request.

Only one extension is allowed for a given type. For example, Password hashing function.

raymondfeng · 2017-10-18T04:44:31Z

Yes, I'm advocating to come up an out-of-box solution for extension point/extension pattern so that we can:

Allow multiple servers for the REST component
Allow multiple strategies for authentication
Allow multiple logging implementations
Allow multiple deserialization/serialization schemes for different media types
...

bajtos

I pushed a new commit fixing markdown syntax in the .md file, please update your local branch before making further changes, @raymondfeng.

The overall goal and design looks pretty good to me.

Besides to comments below, I'd like us to consider what's the right abstraction layer where to implement support for extensions. I personally think @loopback/context should focus on providing a good IoC container only and the concept of extensions/extension points does not belong there, extensions should be a part of @loopback/core, probably implemented in the Application class.

bajtos · 2017-10-18T09:32:59Z

packages/context/test/acceptance/extension-point.ts

+  /**
+   * Find an array of bindings for extensions
+   */
+  getExtensionBindings(): Binding[] {


The difference between names getExtensionBindings and getExtensionBinding (below) is too small and easy to overlook. I am proposing to rename this one to getAllExtensionBindings.

bajtos · 2017-10-18T09:38:33Z

packages/context/test/acceptance/extension-point.ts

+  async getExtension(name: string): Promise<EXT> {
+    const binding = this.getExtensionBinding(name);
+    // Create a child context to bind `config`
+    const extensionContext = new Context(this.context);


We need to carefully consider implications for CONTEXT-scoped bindings, as they will be cached at the level of this temporary extension context. Is this the right thing to do? Shouldn't we cache CONTEXT-scoped bindings at this.context level instead?

Let me think about that.

Meanwhile, I'll evaluate the possibility to make ExtensionPoint extend Context. This way, each extension point will have its own isolated context.

bajtos · 2017-10-18T09:38:56Z

packages/context/test/acceptance/extension-point.feature.md

+
+```ts
+class LocalStrategy implements AuthenticationStrategy {
+  constructor(@inject('config') private config: LocalConfig) {}


+1, this is great.

bajtos · 2017-10-18T09:41:00Z

packages/context/test/acceptance/extension-point.ts

+
+  class AuthenticationManager extends ExtensionPoint<AuthenticationStrategy> {
+    constructor(
+      @inject('$context') context: Context,


I am not very happy that we need to inject the full context object here, but I am not sure if there is anything we can do about that.

bajtos · 2017-10-18T09:42:53Z

packages/context/test/acceptance/extension-point.ts

+    }
+  }
+
+  class AuthenticationManager extends ExtensionPoint<AuthenticationStrategy> {


Using the nameManager is considered as antipattern, see e.g here and here.

Let's use a more descriptive name please, for example AuthenticationHandler.

bajtos · 2017-10-18T09:47:16Z

packages/context/test/acceptance/extension-point.ts

+      .bind('authentication.strategies.local')
+      .toClass(LocalStrategy)
+      .inScope(BindingScope.SINGLETON)
+      .tag('authentication-strategy');


Use tags (all bindings with tag 'authentication-strategy' will be treated as extensions for the authentication.manager extension point.

I think it's a bad idea to have two possibly conflicting conventions for looking up extensions. For example, it allows you to bind authentication strategy as a controller too via ctx.bind('controllers.authentication').toClass(MyStrategy).tag('authentication-strategy'). I know this example is silly, but my concern is that users often do silly things and then we have to spend our precious time explaining them what they did wrong.

I personally prefer to get rid of Binding's .tag() method entirely, considering that we haven't used it anywhere so far, and we are already almost in Beta.

Alternatively, if you insist on supporting both naming convention and binding tags to look up extensions, then please capture this in acceptance tests, including how edge cases like my silly example above should be handled.

bajtos · 2017-10-18T09:48:35Z

packages/context/test/acceptance/extension-point.ts

+  extensions: {
+    [extensionName: string]: any;
+  };
+  extensionPoint: {


What is the point of this member? AFAICT, it's not used anywhere else in this pull request.

I was trying to separate config properties for the extension point itself from the ones for extensions.

bajtos · 2017-10-18T09:49:31Z

packages/context/test/acceptance/extension-point.ts

+    // Register the extension point
+    ctx.bind('authentication.manager').toClass(AuthenticationManager);
+    ctx.bind('authentication.config').to({
+      extensions: {


Can we avoid this extra level of nesting (config -> extensions -> local) and go for shorter config -> local?

bajtos · 2017-10-18T09:52:33Z

packages/context/test/acceptance/extension-point.ts

+    });
+
+    // Register multiple extensions
+    ctx


I'd like to see some kind of helpers to simplify the registration of extensions. Ideally, I'd want the registration code to refer to extension point by constructor (AuthenticationHandler) and then have everything else filled for me by the framework.

For example:

app.bindExtensionOf(AuthenticationHandler).toClass(LocalStrategy);

(I am using AuthenticationHandler instead of AuthenticationManager per my earlier comment.)

bajtos · 2017-10-18T09:57:44Z

packages/context/test/acceptance/extension-point.ts

+  getExtensionBinding(name: string): Binding {
+    const key = `${this.name}.${name}`;
+    const extensions = this.getExtensionBindingMap();
+    return extensions[key];


I am proposing to simplify this method to the following:

return this.context.getBinding(key);

The current implementation builds a full map of all extension bindings, which is a preliminary pessimization IMO.

There is also a subtle difference handling of unknown extension names: the current proposal returns undefined, my proposal throws error The key ${key} was not bound to any value..

kjdelisle

Echoing feedback I gave earlier, it seems a bit verbose, but I can't think of a more terse* way that isn't broken, or doesn't require a rewrite of something fundamental (which would be a mistake, IMO).

LGTM

1. Add context.bindOptions to configure bindings 2. Add context.getOptions to look up options for a binding 3. Add @injection.options to receive injection of options

Extension point/extension is a pattern for extensibility. This PR illustrates how we can support it using the Context apis and naming conventions. It also serves as an invitation to discuss if we should support such entities out of box.

bajtos · 2018-10-26T12:17:37Z

There have been no updates in this pull request for more than a year by now, I am closing it for now. @raymondfeng feel free to reopen if you get time to continue this work. (Considering how outdated these changes are, it may be better to start from scratch.)

raymondfeng requested a review from bajtos as a code owner October 17, 2017 17:35

raymondfeng added review and removed review labels Oct 17, 2017

raymondfeng requested review from kjdelisle and virkt25 October 17, 2017 17:36

raymondfeng force-pushed the extension-point branch from bce8157 to 10fccd6 Compare October 17, 2017 17:44

raymondfeng mentioned this pull request Oct 17, 2017

feat(context): withDependencies binding #631

Closed

2 tasks

raymondfeng force-pushed the extension-point branch 2 times, most recently from 22588d0 to da5315f Compare October 17, 2017 21:01

virkt25 reviewed Oct 18, 2017

View reviewed changes

bajtos reviewed Oct 18, 2017

View reviewed changes

raymondfeng force-pushed the extension-point branch 2 times, most recently from 96b8755 to db0a1d0 Compare October 18, 2017 22:23

raymondfeng changed the title ~~test(context): Add acceptance for extension point/extension~~ [WIP] feat(core): Add acceptance for extension point/extension Oct 19, 2017

raymondfeng changed the title ~~[WIP] feat(core): Add acceptance for extension point/extension~~ [RFC] feat(core): Add acceptance for extension point/extension Oct 19, 2017

raymondfeng force-pushed the extension-point branch 3 times, most recently from 6ba46f5 to 7c67206 Compare October 19, 2017 15:38

kjdelisle approved these changes Oct 19, 2017

View reviewed changes

raymondfeng force-pushed the extension-point branch 8 times, most recently from 24a4b15 to 040dd90 Compare October 19, 2017 22:21

raymondfeng force-pushed the extension-point branch 8 times, most recently from 7d2561f to 2cd34f7 Compare February 12, 2018 22:51

raymondfeng force-pushed the extension-point branch 3 times, most recently from aabdd20 to 2b7268c Compare February 26, 2018 17:51

raymondfeng force-pushed the extension-point branch from 2b7268c to bedf553 Compare March 21, 2018 15:22

raymondfeng mentioned this pull request Mar 22, 2018

Spikes for extension infrastructure #1034

Closed

4 tasks

raymondfeng force-pushed the extension-point branch from bedf553 to 73c6e7e Compare March 30, 2018 22:36

raymondfeng and others added 8 commits April 20, 2018 08:32

feat(context): support binding options and @inject.options

899f376

1. Add context.bindOptions to configure bindings 2. Add context.getOptions to look up options for a binding 3. Add @injection.options to receive injection of options

refactor(context): rename options to config

96e2bab

fix: add type information for getConfig

c1ff7e3

fix(context): add acceptance for configuration

e692000

feat(context): add more utils to resolve valueOrPromises

e2515a9

feat(context): add env specific config support

a55e910

fix(context): use resolveValueOrPromise() to simplify code

d6fad4a

raymondfeng force-pushed the extension-point branch from 73c6e7e to 30fec10 Compare April 20, 2018 16:52

bajtos added post-GA and removed needs-priority labels Jun 25, 2018

dhmlau removed the non-DP3 label Aug 23, 2018

bajtos closed this Oct 26, 2018

raymondfeng mentioned this pull request Nov 2, 2018

Allow extensions to contribute static assets #1967

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat(core): Add support for extension point/extension pattern on top of Context #657

[WIP] feat(core): Add support for extension point/extension pattern on top of Context #657

raymondfeng commented Oct 17, 2017 •

edited

Loading

virkt25 left a comment •

edited

Loading

virkt25 Oct 18, 2017

raymondfeng Oct 18, 2017

bajtos Oct 18, 2017

raymondfeng Oct 18, 2017

virkt25 Oct 18, 2017

bajtos Oct 18, 2017

raymondfeng Oct 18, 2017

virkt25 Oct 18, 2017

virkt25 Oct 18, 2017

raymondfeng Oct 18, 2017

virkt25 Oct 18, 2017

raymondfeng Oct 18, 2017

raymondfeng commented Oct 18, 2017

bajtos left a comment

bajtos Oct 18, 2017

raymondfeng Oct 18, 2017

bajtos Oct 18, 2017

raymondfeng Oct 18, 2017

bajtos Oct 18, 2017

bajtos Oct 18, 2017

bajtos Oct 18, 2017

bajtos Oct 18, 2017

bajtos Oct 18, 2017

raymondfeng Oct 18, 2017

bajtos Oct 18, 2017

bajtos Oct 18, 2017

raymondfeng Oct 18, 2017

bajtos Oct 18, 2017

kjdelisle left a comment •

edited

Loading

bajtos commented Oct 26, 2018


		# TBD

		- Should we go beyond naming conventions to make ExtensionPoint/Extension first class entity of the context?

[WIP] feat(core): Add support for extension point/extension pattern on top of Context #657

[WIP] feat(core): Add support for extension point/extension pattern on top of Context #657

Conversation

raymondfeng commented Oct 17, 2017 • edited Loading

Description

Related issues

Checklist

virkt25 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Oct 18, 2017

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjdelisle left a comment • edited Loading

Choose a reason for hiding this comment

bajtos commented Oct 26, 2018

raymondfeng commented Oct 17, 2017 •

edited

Loading

virkt25 left a comment •

edited

Loading

kjdelisle left a comment •

edited

Loading