DI API Tweak

[ganemone]

I have found myself confusing when to use app.register vs app.configure a few times when working through the DI migration. To give some context, we have two things that can be given to the DI system: plugins and values. A value is given to the DI system via app.configure and a plugin is given to the DI system via app.register. There are a few reasons for this distinction, but suffice to say that it matters.

The problem for me is that app.register intuitively feels like it should work with things that are not plugins. For example:

app.register(FetchToken, window.fetch)
app.configure(FetchToken, window.fetch)

When choosing between the two options above, they both seem equally likely to be correct, when in fact app.configure is correct, and app.register is incorrect.

To fix this, I want to propose a small tweak to the api by renaming app.register to app.plugin.

app.plugin(FetchToken, window.fetch)
app.configure(FetchToken, window.fetch)

This makes it much more clear that only plugins can be registered with the DI system via app.plugin and configuration via app.configure. It also allows us to say "registered with theDI system" without conflating it with app.register, since app.configure is also "registering" something with the DI system.

Along side this change, I think it would make sense to make the following tweaks.

• rename withDependencies to createPlugin
• undo the currying of createPlugin (previously withDependencies)
• remove the need for withMiddleware for pure functional plugins

Lets take a look at some examples of the old vs new apis.

// ----------------------------
// plugin with middleware only
// ----------------------------
// old and busted
const SomePlugin = withDependencies({depA: TokenA})(({depA}) => {
   const myThing = new MyThing(depA);
   return myThing;
});
app.register(SomePluginToken, SomePlugin);

// new hotness
const SomePlugin = createPlugin({depA: TokenA}, ({depA}) => {
   const myThing = new MyThing(depA);
   return myThing;  
});
app.plugin(SomePlugin);

// -------------------------------
// plugin with middleware and api
// -------------------------------
// old and busted
const SomePlugin = withDependencies({depA: TokenA})(({depA}) => {
   const myThing = new MyThing(depA);
   return withMiddleware(myThing, (ctx, next) => next());
});
app.register(SomePluginToken, SomePlugin);

// new hotness
const SomePlugin = createPlugin({depA: TokenA}, ({depA}) => {
   const myThing = new MyThing(depA);
   return withMiddleware(myThing, (ctx, next) => next());  
});
app.plugin(SomePlugin);

// -----------------------
// middleware only plugin
// -----------------------
// old and busted
const MiddlewareOnlyPlugin = withMiddleware((ctx, next) => next());
app.register(MiddlewareOnlyPlugin);

// new hotness
const MiddlewareOnlyPlugin = (ctx, next) => next();
app.plugin(MiddlewareOnlyPlugin);

Overall I think this improves the readability and clarity of fusionjs code and reduces some boilerplate.

The only thing that could still use some work is pure functional middleware with dependencies.

// old and busted
const MiddlewareOnlyWithDeps = withDependencies({depA: TokenA})(({depA}) => { 
  return withMiddleware((ctx, next) => next());
});
app.register(MiddlewareOnlyWithDeps);

// new and still busted
const MiddlewareOnlyWithDeps = createPlugin({depA: TokenA}, (depA) => {
  return withMiddleware((ctx, next) => next());
});
app.plugin(MiddlewareOnlyWithDeps);

This is still a little funky, because nothing is going with the middleware. One option is we could support just returning a function directly, but that would impose the restriction that programmatic apis of plugins couldn't be functions. I'm open to ideas on how to make this better, but I don't think it is a dealbreaker for shipping.

Comments?

[lhorie]

Should MiddlewareOnlyPlugin be wrapped in a function?

// new hotness
const MiddlewareOnlyPlugin = (ctx, next) => next();

// new new hotness
const MiddlewareOnlyPlugin = () => (ctx, next) => next();

[ganemone]

Should MiddlewareOnlyPlugin be wrapped in a function?

I would rather have less unnecessary wrapping things

[lhorie]

Doesn't createPlugin also return a function? How does app.plugin() know when its argument is a middleware vs a plugin?

[ganemone]

Doesn't createPlugin also return a function? How does the system know when the function is a middleware vs a plugin?

We can have it return an object, similar to how it works today in withDependencies

[AlexMSmithCA]

While I am a fan of a single .register(...) for both registering plugins and configuration, I do think this new API is slightly less confusing. Minor enough that I'm not worried about it being a breaking change (adds minimal additional effort on migrations we've already done).

#backToPluginFullCircle

[KevinGrandon]

It seems like two methods will still cause some confusion. We should be able to infer the type of thing you are registering, correct? Just wondering if something like the following is possible:

app.register(TokenType, PluginType | ConfigType)

[ganemone]

@KevinGrandon we could remove configure if we also remove support for registering pure functional middleware without any wrappers (e.g)

// can't do this anymore
const MiddlewareOnlyPlugin = (ctx, next) => next();
app.plugin(MiddlewareOnlyPlugin);

// would need to be 
const MiddlewareOnlyPlugin = withMiddleware((ctx, next) => next()); // or some equivalent wrapper
app.plugin(MiddlewareOnlyPlugin);

[AlexMSmithCA]

In terms of mitigating confusion, I like the idea of a generalized .register that works for any dependency. And only if you want the additional functionality a plugin provides (allows dependencies; middleware...) would the developer need to define it as such somewhere (e.g. createPlugin or createMiddlewarePlugin or withMiddleware, etc...).

[ganemone]

And only if you want the additional functionality a plugin provides (allows dependencies; middleware...)

That is what we need to figure out tho. The big open questions for me are:

1. How do I register a plugin that is a pure functional middleware with no dependencies?
2. How do I register a plugin that is a pure functional middleware with dependencies?

[ganemone]

Potentially we can solve all these problems if we come up with a better name for withMiddleware or split withMiddleware into two functions. The basic idea is we need to either

a. attach a middleware to a plugin
b. denote that the plugin is a pure middleware

With our current api, we accomplish (b) using a special case of (a) by "attaching" a middleware to an empty plugin. In other words:

// (a)
withMiddleware(plugin, (ctx, next) => next());
// (b)
withMiddleware((ctx, next) => next());

My biggest gripe with this is it just feels unnatural. We could rename (b) to something else like createMiddleware but then it becomes confusing regarding which one to use for (a).

[lhorie]

After some discussion, I think the consensus is some variation of:

// all object keys are optional
createPlugin({
  requires: {dep: DepToken},
  provides({dep}) {
    return something
  },
  middleware(deps, something) {
    return (ctx, next) => {}
  }
})

or

createPlugin({dep: DepToken}, ({dep}) => ({
  provides: something,
  middleware: (ctx, next) => {}
}))

Other variations include using the word deps instead of requires and exports instead of provides.

Another suggestion is to have an overload for createPlugin(middleware)for dep-less middlewares.

Registration would look like:

const MyPlugin: Plugin = createPlugin(...); // same API for service plugin, middleware plugin and service+middleware plugin

app.register(SomeToken, MyPlugin);
app.register(SomeConfigToken, 1000);

[ganemone]

I'm pretty sold on option 1, and I'm partial to using deps

[ganemone]

Actually now I'm partial to requires lol

[lhorie]

Final API is:

createPlugin({
  deps: {dep: DepToken},
  provides: ({dep}) => {
    return something
  },
  middleware: (deps, something) => {
    return (ctx, next) => {}
  }
})

app.register([Token, ] PluginOrValue);
app.middleware(middleware);

[lhorie]

On a side note, we are also using a convention of returning a object with interface {from(ctx) {}} for plugins that return something that is meant to be scoped by request.

For example

createPlugin({
  deps: {I18n: I18nToken},
  middleware({I18n}) {
    return (ctx, next) => {
      const i18n = I18n.from(ctx); // get an instance that is scope by request
      i18n.translate('foo');
    }
  },
})