feature request: ability to return a promise from add

[faceyspacey]

Imagine you are setting up a redux container component with data from async action creators--well, then you need support for promises and async/await in the function passed to add.

I would like to make a PR. I'm gonna start looking into the code soon. Can you give me some hints where I should apply this. Thanks in advance.

[ndelangen]

Sounds like an good opportunity! PR is very much welcome!

[usulpro]

@faceyspacey

this is place where you can start:
https://github.com/storybooks/react-storybook/blob/master/src/client/preview/client_api.js

But I wonder if you need a new functionality why don't consider to create an addon for it instead?
Storybook has setAddon API for that. You can take as an example react-storybook-addon-info

Unfortunately, It's missing in the official documentation. I think we will need to update it

[faceyspacey]

if you can of course, but I doubt you can add support for promises to the core api.

it looks like promise support will be needed right here:
https://github.com/storybooks/react-storybook/blob/master/src/client/preview/client_api.js#L67

The story gets added to the StoryStore via this method:
https://github.com/storybooks/react-storybook/blob/c46a341556c61ac2e99bb70ccee951368b1d75f5/src/client/preview/story_store.js#L13

But somewhere else the fn is eventually called. There likely needs to deal with promises as well. Perhaps that is the only place where the promise must be resolved.

...and those places ultimately are here:

https://github.com/storybooks/react-storybook/blob/master/src/client/preview/render.js#L48
story is retrieved from the storybook redux store

and here:
https://github.com/storybooks/react-storybook/blob/master/src/client/preview/render.js#L73
your fn is finally called

and here it the "preview" rendering is complete:

https://github.com/storybooks/react-storybook/blob/master/src/client/preview/index.js#L48

so perhaps just the last 2 calls in the chain need to be made to resolve promises.

...now all that is left is to do it.

[faceyspacey]

Actually you don't need to touch the final handler passed to subscribe. You only need to change renderMain and the initial decorators. Here's renderMain:

export function renderMain(data, storyStore) {
  // ...
  story(context).next(element => { // THIS NOW RESOLVES A PROMISE!
   if (!element) {
    const error = {
      title: `Expecting a React element from the story: "${selectedStory}" of "${selectedKind}".`,
      /* eslint-disable */
      description: stripIndents`
        Did you forget to return the React element from the story?
        Use "() => (<MyComp/>)" or "() => { return <MyComp/>; }" when defining the story.
      `,
      /* eslint-enable */
    };
    return renderError(error);
   }

   if (element.type === undefined) {
    const error = {
      title: `Expecting a valid React element from the story: "${selectedStory}" of "${selectedKind}".`,
      description: stripIndents`
        Seems like you are not returning a correct React element from the story.
        Could you double check that?
      `,
    };
    return renderError(error);
   }

   ReactDOM.render(element, rootEl);
   return null;
 }
}

export default function renderPreview({ reduxStore, storyStore }) {
  const state = reduxStore.getState();
  if (state.error) {
    return renderException(state.error);
  }

  return Promise.resolve(renderMain(state, storyStore))
      .catch(ex => renderException(ex))
}

renderPreview is ultimately passed to reduxStore.subscribe, which means its return isn't used, which means it can be a promise with no negative consequence. See (preview/index.js):

const renderUI = () => {
  if (isBrowser) {
    renderPreview(context); //promise resolved, but return not used :)
  }
};

reduxStore.subscribe(renderUI)

...@usulpro perhaps you wanna give the decorators a try. The idea is that our story function returns an element, and the decorators can wrap it in a container element. That's all. But the wrapping must be synchronously performed. So they must be resolved as a group.

[faceyspacey]

the decorators will be more complicated since it happens in a reduce loop. Basically, I think we should pull out the inner most decorated call, i.e. the one to our story and resolve that, and then in the then block, wrap its result in any other potential decorators. I guess the answer is:

this:

const fn = decorators.reduce((decorated, decorator) => {
   return (context) => {
     return decorator(() => {
       return decorated(context);
     }, context);
   };
}, getStory)

becomes something like this:

const fn = context => {
    return getStory(context).then(storyElement => {
      return decorators.reduce((decorated, decorator) => {
          return () => decorator(decorated);
      }, () => storyElement)
   }
}

note: this assumes decorators don't need the context, but they might. It would actually be a problem if they need the context since we need to now pass it to the story func immediately, and if the decorators expect to immutably pass a new context object, they will not have this chance. However, i searched the code and it doesn't seem like any decorators use it, but perhaps its an option available in userland, so in which case we just pass it to the inner () => decorator(decorated, context) call with the assumption that the decorator won't be able to pass any information through to the main story (since it was already called), but it can pass info to other decorators by using the context mutably. This would be a breaking change, but likely for nobody since it's not even documented that context is passed as a second argument to your decorator functions.

...now we just need to give it a spin.

[faceyspacey]

and it turns out, the Storybook team has been planning on removing the context argument:

#340 (comment)

[ndelangen]

I imagine the API would look like this:

storiesOf('DataViewer')
  .add('test', () => fetch('http://api.someURLtogetdata.com').then(r => r.json())
    .then(({ data }) => <TestStory data={data} />)
  )

[faceyspacey]

@ndelangen what's the latest on async support? I think we are in agreement in what the API looks like, but I don't suppose you could perhaps go over my guesses on how to implement it above or have someone who knows the codebase do so?

I'll likely do it myself in the coming weeks, but it would be helpful to know if I'm on the right rack.

[ndelangen]

This might just do it: https://github.com/storybooks/storybook/compare/async-render-support?w=1

Testing it now..

[ndelangen]

Tested, it works great, story re-renders (and promise is re-resolved every time.)

Do you want to give this a try @faceyspacey ?

[ndelangen]

I made a PR, and will merge it after also adding some documentation and an example in kitchen sink.

Want to add your comments @faceyspacey ?

[faceyspacey]

Wow! I missed this. I'll try to check this out sooner than later.

[dchambers]

Hi @ndelangen,

I also need to be able to return a promise from the add method, so tried updating my copy of node_modules/@storybook/react/dist/client/preview/render.js to match the given diff, but AFAICT the renderMain function in the file I've edited is never ever invoked.

Am I doing something stupid?

[ndelangen]

@dchambers Try this diff maybe? #1253

Hmm, this worked back then..

[dchambers]

Hi @ndelangen, thanks for the help here and sorry I took so long to come back to you.

This diff seems to be an updated but otherwise identical diff to the last one, and the problem I'm seeing is that the renderMain function never seems to be invoked at all.

Scratch that, I've just realized that this code is only used on the client-side, whereas I've been testing within Jest (using storyshot testing). Although it's not currently working for me in the browser either -- I see this when wrapping my story in a Promise.resolve():

Expecting a valid React element from the story: "Available Questions" of "QuestionPanel".
Seems like you are not returning a correct React element from the story.

Could you double check that?

I have a starting point and will try debugging this further tonight...

[dchambers]

Hi @ndelangen, I've just looked at this again and your diff does work for storybook proper, but it looks like a separate change would be needed to get this working for storyshots. I'm actually only really adding the promise for storyshots because otherwise I can't delay when the snapshots are taken, and this causes them being taken before any data has arrived, so that I just see the loading indicator.

I spent a good amount of time trying to get this working on the weekend, but had to work almost blind as a result of the fact that debugging in Jest isn't currently supported with the Node.js inspector (expected to be fixed in 8.4.0) and since console.log() statements were swallowed within the body of the test.

Yay, looks like Node.js 8.4.0 has just been released so will try to debug this now...

[dchambers]

I've now been able to get this working for snapshots too. I created an asyncSnapshot test function that I could use instead of the built-in snapshot function, like so:

const asyncSnapshot = ({ story, context }) => {
  return Promise.resolve(story)
    .then(storyVal => storyVal.render(context))
    .then(storyElement => {
      const tree = renderer.create(storyElement, {}).toJSON();
      expect(tree).toMatchSnapshot();
    });
};

initStoryshots({
  test: asyncSnapshot
});

and then I modified the it function in addons/storyshots/src/index.js to return the promise that I was providing it through my test function, like so:

it(story.name, () => {
  const context = { kind: group.kind, story: story.name };
  return options.test({ story, context });
});

As it happens this doesn't actually help me in my case since the asynchronous delay seems to be within the component itself, so that I need some other way to delay the snapshot from being taken to avoid seeing the loading indicator.

[dchambers]

I've now been able to work around my particular issue by using a delayedAsyncSnapshot test function like this:

const delayedAsyncSnapshot = ({ story, context }) => {
  return Promise.resolve(story)
    .then(storyVal => storyVal.render(context))
    .then(storyElement => {
      renderer.create(storyElement, {});
      return new Promise((resolve, reject) =>
        setTimeout(() => resolve(storyElement), 0)
      );
    })
    .then(storyElement => {
      const tree = renderer.create(storyElement, {}).toJSON();
      expect(tree).toMatchSnapshot();
    });
};

initStoryshots({
  test: delayedAsyncSnapshot
});

Obviously this is probably only useful to me, whereas the asyncSnapshot function given earlier should probably be the default implementation within snapshot going forwards.

[dchambers]

I was actually able to add the delay I need within the test code, so just regular promise support is working great for me. In case it makes it easier to explain, here's a diff for the changes I'm using:

https://github.com/storybooks/storybook/compare/master...dchambers:master?w=1

[tmeasday]

@dchambers I'm glad you are making progress here. Can I ask the reason for the async story in the first place? I've never quite managed to get my head around this particular feature

[ndelangen]

@dchambers Awesome progress you're making!

[dchambers]

Hi @tmeasday, in my case I'd like to use to Storyshots to do snapshot testing of GraphQL backed components. Although I connect to an in-process mock GraphQL server rather than a real one for Storybook and unit testing, Apollo still returns the data asynchronously which means that all of my snapshots end up rendering 'Loading...' rather than the components I've created.

There may well be other scenarios where the use of promises to delay things might help, but this is the concrete issue I'm facing.

I can spend some time tomorrow evening adding some tests and maybe some docs that provide an example that hopefully helps to make sense of this feature.

[tmeasday]

@dchambers thanks for the explanation. That makes perfect sense, and I can understand the use case.

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 60 days. Thanks!

[leojh]

This feature was never completed, correct? There is no way to asynchronously render a story still?

[ndelangen]

Correct 😞

[pelotom]

My use case is that I'm trying to use Storyshots with a component that, several levels down in its render tree, uses a component that is lazily loaded with import(...) from a codesplit module. So on first render it's almost entirely blank. Does anyone have any ideas on the easiest way to get Storyshots to wait until the component is loaded to render? Or to just load the component synchronously if in test mode?

[tmeasday]

@pelotom could you import all the relevant modules in your storyshots (jest) test file in a beforeAll async block? Kind of janky but maybe would work in theory.

[pelotom]

@tmeasday I tried that (actually just a regular static import at the top of the file) but it didn't seem to work. I think even if the module is immediately available the promise won't resolve synchronously.

[tmeasday]

Can you mock `import` to just do nothing?

…

On Mon, 16 Apr 2018 at 12:26 pm, Tom Crockett ***@***.***> wrote: @tmeasday <https://github.com/tmeasday> I tried that (actually just a regular static import at the top of the file, but it didn't seem to work. I think even if the module is immediately available the promise won't resolve synchronously. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#713 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIFyj89v-rjyjgMUMJ2TQ8Zrs519Zb6ks5tpAFNgaJpZM4MVEe6> .

[pelotom]

@tmeasday import doesn’t exist in the transpiled code that Jest is running. It gets turned into promise code which immediately resolves to the result of a require call.

[tmeasday]

@pelotom this may be getting too complicated, but what about using a different babel plugin for import() then? One of these maybe:

https://www.npmjs.com/package/babel-plugin-dynamic-import-node-sync
https://www.npmjs.com/package/babel-plugin-transform-import-to-require

Could that possibly work?

[pelotom]

@tmeasday I'm not using Babel at all in my project (I'm using TypeScript), and would rather not start now. The solution I'm using for now is to pass down an optional override component to use instead of the lazy one if present.

[pelotom]

Lol, why the thumbs down??

[tmeasday]

I mis-clicked ;) Did you get a notification or something? Looks like a thumbs up for me!

[waynebrantley]

@tmeasday @dchambers This is still an issue with storybook and snapshot testing.
Based on the source code I have created a modified version of snapshotWithOptions. This modified version make sure fetch-mock has finished its work and that all promises are finished before comparing the snapshot to the tree. This is a copy and paste of existing source code with two lines added.

If an extension point of 'beforeSnapshotComparison' was provided in the existing code, everyone could do this. In addition, the code that waits on promises to finish could probably just be inserted no matter what (where fetch-mock is developer specific)

const snapshotWithOptions = (options = {}) => ({
    story,
    context,
    renderTree,
    snapshotFileName,
}) => {
    const result = renderTree(story, context, options)

    const match = async (tree) => {
        await fetchMock.flush()   //new code
        await new Promise((resolve) => setImmediate(resolve))   //new code
        if (snapshotFileName) {
            expect(tree).toMatchSpecificSnapshot(snapshotFileName)
        } else {
            expect(tree).toMatchSnapshot()
        }

        if (typeof tree.unmount === "function") {
            tree.unmount()
        }
    }

    if (typeof result.then === "function") {
        return result.then(match)
    }

    return match(result)
}

const asyncMultiSnapshotWithOptions = (options = {}) => ({
    story,
    context,
    renderTree,
    stories2snapsConverter,
}) =>
    snapshotWithOptions(options)({
        story,
        context,
        renderTree,
        snapshotFileName: stories2snapsConverter.getSnapshotFileName(context),
    })

Thoughts?

[andwaal]

@waynebrantley I have struggeling with the excact same issue. All my components follows the same pattern, where all data is loaded in componentDidMount(which I mock using fetch-mock in stories), resulting in empty snapshots since the snapshot is captured before the data is done loading.

So your proposal of having an beforeSnapshotComparison extention seems like a great solution for this issue!

[waynebrantley]

@andwaal if you use the above it will help you do exactly what you want - at the expense of just some copied code. Hopefully, they can just add the extension (with the awaits, etc), but if not - it works great now.

[dtb]

This comment from #696 offers a simple workaround that worked for me.

(sorry for the noise, but this issue is the first result in Google)

[VladimirCores]

And how to apply storiesOf to Vue component?