Proposal: Addons composition

[usulpro]

Issue:

#766 and other discussions

1. We want to apply not only one addon for a story
2. Addons like .addWithX don't allow addons composition

What we have at this moment:

1. We started to move from addons like .addWithX to addons like withX()

withX() in general has such API:

withX(...props) { /* return storyFn(context) => <Story /> */ }

At this moment we have withInfo and addonNotes. And we can compose them this way:

.add('addons composition',
    withInfo('with info')(addonNotes({ notes: 'with notes' })(() => <button>click me</button>))
)

It's much better but we still have issues with this:

a. It looks like a kind of "callbacks hell" when we need to compose several addons

b. withX() should have such structure:

const withX = (...props) => {
  return getStory => (context) => {
    return getStory(context);
  };
};

2. We have current API for .add()

storiesOf(...)
  .add(name, (context) => <Story />)

where context is an object: { story, kind }. Usually, we omit context: add(name, () => <Story />)

and decorators API is:

storiesOf(...)
  .addDecorator((storyFn, context) => { /* return storyFn(context) */ })

What we want:

1. Convenient and flexible way for applying addons

2. Easy approach for creating new addons

Proposal:

1. Use this way for addons applying:

storiesOf('Button', module)
  .add('with addons', context =>
    context
      .storyOf(<Button>Press me</Button>)
      .withBorder('red')
      .withNotes('Addons composition')
      .withInfo('Addons composition', { inline: false })
  )
  .add('single story', () => <Button>Hello Button</Button>)

It shouldn't break the existing API and should be backward compatible.

Actually, this approach still keeps the same API:

storiesOf(...)
  .add(name, (context) => <Story />)

2. Start creating addons functions with such structure:

const withX = (storyFn, context, ...props) => { /* return storyFn(context) */ }

Here withX just returns new component and do some side effects.

3. Possibly we don't want to expand storybook API surface and change Storybook code as well ;-)

What I did:

I just created an addon to implement this behavior! :)

It works this way:

// config.js
import { configure, setAddon } from '@storybook/react';
import getAddons from './addon-composition';

setAddon(getAddons);

// stories.js
import { withInfo } from '@storybook/addon-info';
import { withNotes } from '@storybook/addon-notes';

// Let's create a simple addon as an example:
const withBorder = (storyFn, context, bcolor) =>
  // the first two arguments should be storyFn and context
  <div style={{ padding: 6, border: `${bcolor} solid 2px` }}>
    {storyFn()}
  </div>;

storiesOf('Button', module)
  .getAddons(withInfo, withNotes, withBorder) // <-- pass addons we want to use
  .add('with addons', context =>
    context
      .storyOf(<Button>Press me!</Button>) // <-- the story and addons below:
      .withNotes('Addons composition')
      .withBorder('red')
      .withInfo('Addons composition', { inline: false })
  )
  .add('with text', () => <Button>Hello Button</Button>) // <-- usual stories
  .add('with some emoji', () => <Button>😀 😎 👍 💯</Button>)

In this example we create a <Button> and applying addon in the following order:

• withNotes
• withBorder
• withInfo

We can add normal stories to the same storiesOf - they will work as usual

The pros of this approach

1. It's backward compatible with existing API

2. Easy to compose addons, change addons order, disable/enable addons.

3. Addons with such structure could be easily turned to decorators:

storiesOf('Button', module)
  .addDecorator((storyFn, context) => withX(storyFn, context, ...props))

4. We can create a composition of not only addons but stories as well. For that we can pass a function to context.storyOf it will be invoked with an argument prevStory - the result of previous execution of context.storyOf:

storiesOf('Button', module)
  .getAddons(withInfo, withNotes, withBorder)
  .add('with addons', context =>
    context
      .storyOf(<Button>Press me!</Button>)
      .withJSX() 
      .storyOf(prevStory => 
        <div> 
          Press this button: 
          {prevStory} 
        </div> 
      ) 

In this example withJSX() shows details of <Button> while we see the button wrapped into div. Even if withJSX() change the story somehow, prevStory will be the clean value passed directly from previous storyOf. I guess it's a quite unusual feature and may be discussed in a separate issue.

How to test

Run cra-kitchen-sink
Play with the example:

• change an order of addons
• put addons to decorators
• write your own addon :)

src of addon:
examples/cra-kitchen-sink/.storybook/addon-composition.js

example:
examples/cra-kitchen-sink/src/stories/index.js

Additional

I guess one point could confuse in this addon implementation:

storiesOf('Button', module)
  .getAddons(withInfo, withNotes, withBorder) // <--

if we don't want to do it for every storiesOf maybe more convinient could be to do it ones in config.js:

// config.js
import { configure, setAddon } from '@storybook/react';
import { withInfo } from '@storybook/addon-info';
import { withNotes } from '@storybook/addon-notes';
import getAddons from './addon-composition';

setAddon(getAddons(withInfo, withNotes)); // <--

but for that, we need a feature in setAddon API to execute a function when storiesOf is creating (something like 'constructor' for classes). This feature is introduced in this PR: #1463

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (release/3.3@852ee89). Click here to learn what that means.
The diff coverage is 0%.

@@              Coverage Diff               @@
##             release/3.3    #1473   +/-   ##
==============================================
  Coverage               ?   22.43%           
==============================================
  Files                  ?      236           
  Lines                  ?     5341           
  Branches               ?      751           
==============================================
  Hits                   ?     1198           
  Misses                 ?     3598           
  Partials               ?      545

Impacted Files	Coverage Δ
addons/notes/src/index.js	0% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 852ee89...33769c9. Read the comment docs.

[usulpro]

@tmeasday, @shilman, @ndelangen, @wcastand, @tuchk4
everybody using/writing addons
your feedback would be appreciated!

interesting to test it with readme and jsx addons

It's a proposal and I guess we have options:

• if we like this feature as addon we can publish separate addon

• if we don't like it at all we can close this PR

• if we like it much we can have it as default behavior

[tmeasday]

Hey @usulpro, interesting proposal!

Although it's maybe similar to storybook's API already, I'm not really a huge fan of jquery style chained APIs. I tend to think just a pure functional API is simpler to understand and I think the API awkwardness you mentioned can be resolved in a couple of ways:

1. At the moment, with no changes to storybook, users can just use existing tools (<- this is a plus to doing things the way the React community is moving) to neaten things up:

import { compose } from 'recompose';

// ...
.add('addons composition',
  compose(
    withInfo('withInfo'),
    addonNotes({ notes: 'with notes' })
  )(() => <button>click me</button>)
);

2. Also, dealing with functions just makes it much simpler for people to combine things adhoc:

function withInfoAndNotes(notes) {
  return compose(
    withInfo(notes),
    addonNotes({ notes: notes })
  );
}

.add('addons composition', withInfoAndNotes('my note')(() => <button>click me</button>));

3. It would be a tiny change to our API to explicitly support composing decorator functions like the above to make the example even simpler:

// There have been a few suggestions, this seems a simple one:
.add('addons composition',
  [withInfo('withInfo'), addonNotes({ notes: 'with notes' })]
  () => <button>click me</button>
);

(The above is a simpler variant of my addon proposal #1212 (comment))

---

We should revive that discussion, I think we need to collectively make a decision on this API to move forward.

[wcastand]

Hello,

I'm not a fan of this approach, i already explained my thoughts multiple times so i just link the issue that explain my point of view. #1212

If i had to choose, i'll definitly go with @tmeasday proposal of using recompose.

[usulpro]

@tmeasday, thank you for your response! Totaly agree that we need to move forward, actually, it's the reason of this post. I've studied your proposal and tried to find the simplest implementation for your ideas.

tl;dr

What do you think if we take your third example and turn it to this:

.add('addons composition',
  [
   withInfo('withInfo'),
   addonNotes({ notes: 'with notes' }),
   () => <button>click me</button>,
]);

Details:

It could take advantages of both approaches and still has tiny changes to API

In my previous example, I passed to addons additional prop cleanStory (the story not modified by other addons). It's relevant to your idea of "inspectors", but with the difference that it's up to addon what to use "clean" story or "changed" story. I guess "inspecting" addons should work with "clean" story while "modifiers" and "wrappers" with "changed" story. How do you think can we still use this approach?

In this case, the order of "inspectors" doesn't matter and they even could go mixed with other types of addons. I guess the order of "modifiers" and "wrappers" always matter, so we can't do anything with it.

At the same time maybe it worth to determine that "addons" are only "inspectors". and "decorators" are only "modifiers" or "wrappers" (with the same API though). So we can have a convention that "addons" shouldn't "pollute" the story. And does it sense that "decorators" should have the possibility to be applied both as a "per-chapter decorators" (.addDecorator) and as a "per-story decorators"? So that's why I offer this simple structure for addons/decorators: (...props?) => { /* return storyFn() */ }

[usulpro]

@wcastand doesn't you raise here #1212 (comment) the similar question of "polluted" stories?

Each middleware should receive the same story

[wcastand]

If i had to choose one API proposal, i'd choose mine obviously x) but with the two choices you made on this thread (yours and tmeasday' s).
I found the tmeasday's solution cleaner in term of code.

Doesn't mean i think it's a good solution.
And your solution doesn't separate decorators and inspectors and the WithXapproach pollute your story by default because he put a wrapper around it :)

How would you manage the different level for adding addons:

• like to all the stories
• only the stories of Button
• to only the second story of Button ?

My main problem with your solution is that is backward compatible i guess. I think it's a bad idea to keep it backward compatible in the case of addon in storybook. Because right now it's a mess and it's not clean (for dev and users) which addons does what, which addons modify or not your story, ...

A clear break, with clear guidelines and boundaries for addons will benefit storybook in the long run more than making something that works with the previous API because it mean even more ways to do the same things and no real "rules".

That's how i see things, i know i'm pretty lonely on this one but i still think it's a good way of thinking about this. I don't think being backward compatible is always the best thing :).

Plus, not backward compatible doesn't mean, we leave you with your old code and have fun migrating to the new one. We can provide migration guide, codemods, tutorials, and help the transition. React does it pretty well, webpack too.

[shilman]

I'm always of the opinion that we shouldn't change the API unless there is some benefit to it (something we could do that we couldn't do before), so I'm in favor of the simplest proposal, which requires only that we standardize on withX as a common interface for addons:

// ...
.add('addons composition',
  compose(
    withInfo('withInfo'),
    withNotes({ notes: 'with notes' })
  )(() => <button>click me</button>)
);

@tmeasday and I discussed ways that we can make this less verbose in the future, for example:

registerAddon({ info: withInfo, notes: withNotes })
storiesOf('path/to/Component', module)
  .add('story', () => <Component />, {
    info: 'some info',
    notes: { notes: 'some note', option1: 'some other option' }
  })

The "benefit" here being that we've discussed the need for adding options to individual stories in other places and we are reusing that functionality here to make the addon configuration for each story less verbose.

We could further standardize on on withX to receive a single object argument with named fields.

For me the priority is to create a common convention and simply use compose to start out with. I don't understand the rush to reinvent the addon API without clear functional benefits.

[tmeasday]

There are two problems with addon/decorators that need to be solved, if we assume that we are moving toward a world where addons export a decorator on the preview side (this is the withX direction).

1. Applying decorators at the individual story level in a less verbose way.
2. Controlling the order of decorator application.

I think the two problems are fairly separate (although not completely, of course). Perhaps in this issue we should stick to problem 1. above -- @usulpro I'm not quite sure I understand what you are saying about problem 2, but maybe let's discuss elsewhere (on #1212?)

---

So, considering just the problem of "how do I apply (multiple) decorators at the level of a story?", there seem to be a few approaches:

1. Simply wrapping the story HOC-style (this is already possible):

.add('...', A(args)(B(args)(() => <story/>)));

2. Using compose to bring the brackets under control (also already possible):

.add('...', compose(
  A(args),
  B(args)
)(() => <story/>));

3. Adding a "compose style" story API (as suggested by @UsulPro):

.add('...', [
  A(args),
  B(args),
  () => <story/>,
]);

4. Something similar where the decorators are separated from the story (my strawman in this post), or (with order changed) - @wcastand's proposal 1:

.add('...', [
  A(args),
  B(args),
], () => <story/>);

5. Being explicit about the above via a decorators option (keep in mind that a third argument for options to .add seems pretty likely to happen) - @wcastand's proposal 2:

.add('...', () => <story/>, {
  decorators: [
    A(args),
    B(args),
  ]
});

6. Short circuiting the above via some "registration" API (as @shilman and I discussed):

// In `.storybook/config`
registerDecorators({
 a: A,
 b: B,
});

// In each story
.add('...', () => <story/>, {
  a: args,
  b: args,
});

7. The original API of this PR, a "chained" version:

  .getAddons(A, B)
  .add('with addons', context =>
    context
      .storyOf(<story/>)
      .withA(args)
      .withB(args);
  )

---

Phew! I'm not sure which is best but it's good to see them all here.

[mrmartineau]

FWIW, having needed multiple addons that use the withX method, my preferred solution from the above proposals is number 7. Chaining, while not the most functional way to do this, is certainly the most natural way to do it.

Having introduced Storybook (with a bunch of addons) on a project recently, it definitely would have made my life easier if the addon API was like the chaining proposal.

Having said that, for addons like withJSX, I would much rather set that once for all stories in the addons.js file rather than on each story.. Not sure if that negates the points above or not..

[ndelangen]

This doesn't look like a good idea to me.

[ndelangen]

This is not really an api I'm really enthusiastic about to be honest.

I don't like it because the interface is modified by addons. I fear that this is too flexible, and can lead us to all sorts of problems.

[danielduan]

Closing this because there hasn't been any activity here for a while. Feel free to reopen when this is ready for discussion again.