fusionjs / fusion-core

Migrated to https://github.com/fusionjs/fusionjs
MIT License
630 stars 45 forks source link

Move/improve createToken api into fusion-core #102

Closed ganemone closed 6 years ago

ganemone commented 6 years ago

We have a few issues with the current createToken api which would be solved by some small changes.

  1. Move createToken into fusion-core

This allows fusion-core to define more about the shape of a token which will in turn lead to better error messages. See #93

  1. Update the API of createToken to the following:
const LoggerToken = createToken('Logger');

// require a logger
createPlugin({
  deps: {
    logger: LoggerToken.required, 
  },
});

// optional logger
createPlugin({
  deps: {
    logger: LoggerToken.optional, 
  },
});

Thoughts?

mlmorg commented 6 years ago

I see the benefit of (1) for improving error messages but can you explain the reason/benefits for making the change in (2)?

Can you add a section to discuss how this will be rolled out? Something in the style of https://github.com/MithrilJS/mithril.js/blob/next/.github/PULL_REQUEST_TEMPLATE.md would be helpful, I think.

ganemone commented 6 years ago

The benefit of (2) is that plugins don't need to agree on whether a dependency should be required or not, or what the default is. In the example above, we have three different plugins, all of which declare a dependency on the LoggerToken, however only one of them requires a logger. The bottom two examples have logger as optional.

With the api we have today, we would need three tokens to do the same thing.

const RequiredLoggerToken = createToken('Logger');
const ConsoleDefaultLoggerToken = createOptionalToken('Logger', console);
const OptionalLoggerToken = createOptionalToken('Logger', null);

Then the user would need to register all three of these separately.

ganemone commented 6 years ago

Regarding rollout, we should be able to codemod this pretty effectively.

KevinGrandon commented 6 years ago

Would it be possible/valuable to split this up into two different discussions and PRs? There was another issue tracking optional dependencies: https://github.com/fusionjs/fusion-core/issues/95

ganemone commented 6 years ago

Moving discussion of optional dependencies to #95