Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Add compound plugins #281

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add compound plugins #281

wants to merge 1 commit into from

Conversation

saspre
Copy link

@saspre saspre commented Sep 3, 2018

This adds a feature to create plugins where multiple values can be added to the same token and then be injected into the dependee as an array.

Usage example

 const ArrayToken = createArrayToken('TestArrayToken');
  const plugin = createPlugin({
    deps: {arr: ArrayToken},
    provides: ({arr}) => {
      //arr = [1, 2, 3]
    },
  });
  app.register(
    ArrayToken,
    createPlugin({
      provides: deps => {
        return 1;
      },
    })
  );
  app.register(ArrayToken, 2);
  app.register(ArrayToken, 3);
  app.register(plugin);

Use case
We already use a similar approach to handle apollo resolver registration, though not as generalized as here.
Our use case might be a little specialised as our app contains multiple high level modules that can extend the core functionality, but we do not want to import directly from the modules in the core.
One such feature is custom permission checkers. Each module can add permission checkers for their custom types which will then be run every where they are needed.

Design decisions
There are several ways this could be implemented, I decided to not add an extra method to the fusion app but instead make the tokens special.
I did consider having a addTo or similarly named method for registration.

Documentation
I did not any documentation for this yet, but will add if this is a feature that can be generally used.

@ganemone
Copy link
Contributor

ganemone commented Sep 5, 2018

This is an interesting idea... but I would like to try and keep the API surface area of fusion as small as possible. This could be done with the existing api using app.enhance.

 const ArrayToken = createToken('TestToken');
  const plugin = createPlugin({
    deps: {arr: ArrayToken},
    provides: ({arr}) => {
      //arr = [1, 2, 3]
    },
  });
  app.register(ArrayToken, [1]);
  app.enhance(ArrayToken, (prev) => [...prev, 2]);
  app.enhance(ArrayToken, (prev) => [...prev, 3]);
  app.register(plugin);

@saspre
Copy link
Author

saspre commented Sep 7, 2018

I agree the surface area should be as minimal as possible. This only adds a new method you don’t have to use. The surface area of the FusionApp is unchanged.
The case presented with binding numbers is a rather trivial one, and can easily be solved using enhance as you mention.
The actual problem we are trying to solve is binding multiple “real” plugins with dependencies to the same token in order for them to be injected into another plugin. For example we need to bind multiple GraphQL resolvers to a single resolver registry plugin.

More generally, this feature provides an easy way to implement patterns like composite or strategy with full IOC. For example, the cleanest way to implement the strategy pattern would be to not have any direct coupling between strategies and context, but only rely on a common interface for strategies.

For the GraphQL resolver case we implemented a solution somewhat like this:

this.register(ResolverToken, [])
this.enhance(ResolverToken, prev => { 
return createPlugin({
   deps: {},
   provides: () => {
      const pluginContent = …
      return [...prev, pluginContent];
   },
  });

However, this is messy as you put knowledge into the resolvers about how they are consumed, which is unrelated to the function they actually fulfil. You can wrap the above in a utility function (which we do) but we believe the pattern is general enough (we already have other uses for it in our project) that Fusion should support it directly.

@ganemone
Copy link
Contributor

Chatted with Rasmus offline about this problem and he brings up lots of good points. I think we should consider various options for handling plugins that need to compose with each other. The current approach we have been using is to handle composition in user code. For example,

app.register(TokenA, ThingA)
app.register(TokenB, ThingB)
app.register(ActualToken, createPlugin({
  deps: { a: TokenA, b: TokenB },
  provides: ({a, b}) => compose(a, b)
}))

While a bit verbose, this generally works ok. However it has a few downsides. This works well if you are grouping your plugins by type, (i.e. all redux related things colocated) but falls apart a bit when grouping by feature (for example, if you want to put TokenA/ThingA with all A related things, and TokenB/ThingB with all B related things).

My main problem with the current approach with the ArrayToken is it departs from the expected behavior of app.register with respect to re-registration. I think we should try and come up with an API which allows for applications to solve this problem while adhering to the following principles

  • Type safe
  • pure (should not depend on previous app state)
  • lazy (should not be applied until dependency resolution)

One potential option is to support app.compose and Token.compose. (or some variation on this which there are many). A quick example:

import compose from 'just-compose'
app.compose(Token, A);
app.compose(Token, B);
app.register(Token.compose, (items) => compose(items))
// or similarly
const TokenA = createComposeToken('A', compose);

This is a brief look at one option which leaves out many details such as how this interacts with app.register, testing/mocking, etc. We should do a more thorough investigation into potential options in an RFC.

@lhorie
Copy link
Contributor

lhorie commented Sep 25, 2018

I'm generally against the idea of adding special semantics to do things that can be accomplished with core language features. If you want each value to be overridable, that's the only time you'd need to create tokens for them.

const plugin = createPlugin({
  deps: {a: A, b: B, c: C},
  provides: ({a, b, c}) => {
    const arr = [a, b, c] // 1, 2, 3
    // ...
  },
});
app.register(A,
  createPlugin({
    provides: () => 1,
  })
);
app.register(B, 2);
app.register(C, 3);
app.register(plugin);

If you want one-off values, you can either hard-code them or use a factory pattern

// hard-coded (in provider)
const plugin = createPlugin({
  deps: {a: A},
  provides: ({a}) => {
    // a = [1, 2, 3]
  },
});
app.register(A,
  createPlugin({
    provides: () => [1, 2, 3],
  })
);
app.register(plugin);

// hard-coded (in consumer)
const plugin = createPlugin({
  deps: {a: A},
  provides: ({a}) => {
    const arr = [a, 2, 3]
  },
});
app.register(A,
  createPlugin({
    provides: () => 1,
  })
);
app.register(plugin);

// factory (encapsulating provider)
const plugin = createPlugin({
  deps: {a: A},
  provides: ({a}) => {
    // a = [1, 2, 3]
  },
});
const a = (b, c) => createPlugin({
  provides: () => [1, b, c],
});
app.register(A, a(2, 3));
app.register(plugin);

// factory (encapsulating consumer)
const plugin = (b, c) => createPlugin({
  deps: {a: A},
  provides: ({a}) => {
    const arr = [a, b, c] // 1, 2, 3
  },
});
const a = createPlugin({
  provides: () => 1,
});
app.register(A, a);
app.register(plugin(2, 3));

Making a special ArrayToken has some other problems, namely the need for extra code in core to handle this special case, and need for extra configurability in tokens (assuming we can have any number of ArrayToken-like tokens)

Another issue is that I'm not sure how one can override the token for testing if calling app.register pushes to a list instead of overriding. One possibility would be to use enhance, but that seems like abusive API usage.

We already have cases where token registration reads from a factory (e.g. createI18nLoader). I'd rather follow this pattern since it keeps the type semantics simple, rather than introduce more complexity to cater to a niche use case.

akre54 pushed a commit to akre54/fusion-core that referenced this pull request Dec 3, 2018
@saspre
Copy link
Author

saspre commented Dec 5, 2018

The factory pattern described in @lhorie example does not solve the problem we are facing. The compound tokens might be niche, but I still think they have merit.
The one thing we try to avoid with this is having a single registry of all the plugins registered with the token. This is what we get when using the factory pattern.

Our app is rather modularised, meaning that it contains several parts which are somewhat pluggable. Therefore having to register a modules parts several places will make it the separation very brittle.
Having the ability to easily extend something is very beneficial. This is also generally nice when you use a feature-based folder structure where everything related to the feature is located in one place and not referenced in multiple places across the app.

@lassedamgaard
Copy link

Circling back on this as uses for it keep popping up in our project. To reiterate, the general pattern we want to support is where one “delegator” plugin provides a service via multiple "handler" plugins that implement a common interface.

Currently it does not seem like there's a way to do this in a really modular and loosely coupled fashion which is otherwise something Fusion supports well.

By modular and loosely coupled I mean that:

  1. "Handlers" should not know know anything about “delegators” directly, only the interface they need to implement.
  2. A “delegator” should not reference handlers directly, both because there can potentially be a large number of handlers and because handlers can then be added/removed just by changing the registration with Fusion. This also makes testing the “delegator” easier.

The example of registering GraphQL resolvers with Apollo has already been brought up. Another concrete example from our app is where we show a unified list of “events” originating from different downstream systems. When the user selects a single event we need to query one of those downstreams for the details, based on an identifier in the event ID. The nicest way to structure this is to have a handler for each type of downstream and just let the GraphQl query resolver run through each one until it finds one that can supply the details of that type of event. Yes, you can achieve this today in Fusion via the enhance, but it’s not very elegant and creates a harder coupling between things.

I do not think the general pattern is really that niche, and I also don't think adding this feature is taking the Fusion API down any slippery slope as I find it hard to see what other kinds of “array like” tokens might be added: Fusion currently supports 1:1 bindings and we are simply proposing that it should also support 1:* bindings.

For context, AFAIK many of the bigger DI frameworks for Java have this feature. As an example, the doc for "multibindings" in Guice (https://github.com/google/guice/wiki/Multibindings) provides a good illustration of why this pattern can be useful.

If you think the specific solution proposed in this PR has issues, would you guys be open to discussing it in an RFC as Giancarlo mentioned?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants