Allow promise to be returned from 'add()'

[dchambers]

FWIW, this is a partial fix for #713.

There are no docs, I haven't promisified the other test functions in storyshot (only snapshotWithOptions), and there are no unit tests.

[kanzelm3]

+1 this feature would be useful

[codecov]

Codecov Report

Merging #1670 into release/3.3 will decrease coverage by 1.68%.
The diff coverage is 0%.

@@               Coverage Diff               @@
##           release/3.3    #1670      +/-   ##
===============================================
- Coverage        23.34%   21.65%   -1.69%     
===============================================
  Files              277      252      -25     
  Lines             6041     5717     -324     
  Branches           709      704       -5     
===============================================
- Hits              1410     1238     -172     
+ Misses            4133     3957     -176     
- Partials           498      522      +24

Impacted Files	Coverage Δ
app/react/src/client/preview/render.js	0% <ø> (ø)	⬆️
addons/storyshots/src/index.js	0% <0%> (-80.86%)	⬇️
addons/storyshots/src/utils.js	0% <0%> (-100%)	⬇️
...s/stories/required_with_context/Welcome.stories.js	0% <0%> (-100%)	⬇️
app/react/src/server/config/babel.js	0% <0%> (-100%)	⬇️
...dons/storyshots/stories/directly_required/index.js	0% <0%> (-100%)	⬇️
...ts/stories/required_with_context/Button.stories.js	0% <0%> (-100%)	⬇️
app/vue/src/server/utils.js	0% <0%> (-53.58%)	⬇️
addons/storyshots/src/require_context.js	0% <0%> (-45.59%)	⬇️
... and 110 more

---

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 f396a49...eb658f1. Read the comment docs.

[ndelangen]

Code looks good

not tested yet

Please resolve linting errors

[ndelangen]

Hey @dchambers code looks good to me, and with a clear use-case described here I think we can get this rolling.

I think a unit-test or 2 to test this, a note of this in the docs somewhere would be nice.
An example usage in a kitchen-sink (that is snapshotted) is required to get this merged.

[ndelangen]

We need an example usage that is snapshotted in a kitchen sink

[dchambers]

Hi @ndelangen, I've taken quite a good look at the tests that currently exist and can't find anything that looks like it would be able to verify this new functionality. I guess part of the problem is that what's there at present seems to be more like unit tests rather than BDD tests that test the actual behaviour, and in this case unit testing that some promises were added wouldn't seem to really prove anything.

If I were to add someting then the app/react/src/client/preview/client_api.test.js test would seem to be the place for a test of add() though I would have thought? Any ideas here?

Regarding docs, I have now added a description of the feature that will hopefully be helpful to people.

Thanks again!

[ndelangen]

Can you revert these lines?, sorry, it's not your fault, the markdown linter needs a patch, But I haven't found the time to understand what changed/broke.

[ndelangen]

I think the fastest way to get coverage and confidence is to add a story to a kitchen sink that actually uses this feature!

[ndelangen]

@dchambers Are you interested in finishing this PR by processing the review comments I left for you? Anything you need from me? Anything unclear?

Please let me know, I can take over if you'd like.

[dchambers]

Hi @ndelangen, yes apologies, I've been crazy busy with work the last two weeks, including this weekend. I'm fairly confident I'll be able to make some time to work through the issues you've identified on Monday, if not Sunday.

Thanks, Dominic 👍

[dchambers]

Apologies again for not progressing this at the start of this week as promised. I've still been crazy busy with work. I've now definitely got time to dedicate to this on Friday and Saturday though, so will get everything resolved then. Thanks for your patience on this one 👍

[ndelangen]

Rebased for you 👍 , can you check everything is as it should be?

[dchambers]

Rebased for you 👍 , can you check everything is as it should be?

Thanks @ndelangen. It's not quite working yet, but I've just pushed an attempt at a kitchen-sink that is hopefully in the right direction.

[Hypnosphi]

Why a separate kitchen-sink?

[dchambers]

Why a separate kitchen-sink?

Good point @Hypnosphi, some storybook tests where one of the stories uses a promise could just as easily be added to the existing cra-kitchen-sink. @ndelangen, do you have an opinion here?

One question though, unlike some of the other kitchen-sinks, the CRA one appears to be testing versioned releases of storybook (e.g. "@storybook/addon-actions": "^3.2.0", within package.json), rather than the repo itself. Is this intentional, or is there some magic here to make it test the current repo?

[Hypnosphi]

This magic is called lerna: https://lernajs.io
It links multiple packages between each other when you run yarn bootstrap -- --core

We prefer to keep all examples in one kitchen-sink

React Native examples are a bit special, because they use RN-specific packager. That’s why they are excluded from lerna setup:
https://github.com/storybooks/storybook/blob/e1fbd093121f3074a0d80948bf6765a9fa8cbbfc/lerna.json#L4-L9

[ndelangen]

This is broken in combination with decorators... Looking for a possible solution.

[ndelangen]

Found a solution, I think

[ndelangen]

So the decorator stuff needs to be reviewed and tested really carefully!!!

There's a few alternatives:

• Render stories twice to detect if it's an async story
• have an api specific for async stories
• do not support decorators for async stories (harder than you think)

What's the risk here?
Well this changes the order of execution of storyFn and decorators.
the storyFn gets the context object earlier then before this change, the decorators get the context object after. I'm unsure what the impact of this really is.
Context has always been -blegh- to me.

What this would like break/change in behavior is if someone was doing this: (which I haven't seen anyone do)

storiesOf('Aliens')
  .add(context => <Alien name={'jimmy'} stuff={context} />)

Please advice @Hypnosphi @storybooks/team

[ndelangen]

Obviously unit-tests needs fixing / storyshots needs a fix.

I hope we can have some more eyes looking for on the potential impact on this!
And then I or @dchambers can fix them.

[tmeasday]

Is this something we should be considering alongside the mooted changes to the addon API?

[Hypnosphi]

What was the initial point of passing story to decorators as a function instead of just

addDecorator(story => <wrap>{story}</wrap>)

Maybe it's supposed that the decorator can somehow affect that function's output before calling it?

[Hypnosphi]

Maybe in 4.0.0 we should introduce some redux-middleware-like API for decorators:

addDecorator(context => next => story => ...)

Then async stories could be supported with promise decorator:

addDecorator(context => next => story => Promise.resolve(story).then(next))

[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.

[breathe]

Having trouble following this -- is there still no support for any form of async story definition/rendering? This feature is vital ...

[leojh]

I got confirmation that this feature was never finished. What is the appetite to get this effort started again? Or is there a plan for this to be inherent in the v4 release?

[husayt]

I have been trying this in 5.2 and it seems this is still not supported. Seems there is an alternative constructs available for using async data in stories. Maybe someone could suggest some directions?

This is what I tried and it doesn't work as add doesn't support promises

storiesOf("Article", module)
  .add("test", async () => {
    let dt= await getDynData("article")
    return {
      components: { Article },
      data() {
        return {
          art: {
            link: "test",
            title: dt,
            desc: "test",
          }
        }
      },
      template: `<Article v-bind="art" />`
    }
  })

[Hypnosphi]

@husayt in 5.2-beta, you can try using Storybook Hooks. They a similar to React Hooks but work with any framework:

import {useState, useEffect} from '@storybook/preview-api'

storiesOf("Article", module)
  .add("test", () => {
    const [dt, setDt] = useState('Loading...')
    useEffect(() => {
      let mounted = true
      getDynData("article").then(result => {
        if (mounted) {
          setDt(result)
        }
      })
      return () => {
        mounted = false
      }
    }, [])

    return {
      components: { Article },
      data() {
        return {
          art: {
            link: "test",
            title: dt,
            desc: "test",
          }
        }
      },
      template: `<Article v-bind="art" />`
    }
  })

You may want to extract a custom hook to reuse this kind of logic:

function useAcyncData(getData, placeholder) {
  const [data, setData] = useState(placeholder)
  useEffect(() => {
    let mounted = true
    getData().then(result => {
      if (mounted) {
        setData(result)
      }
    })
    return () => {
      mounted = false
    }
  }, [])
  return data
}

storiesOf("Article", module)
  .add("test", () => {
    const data = useAsyncData(() => getDynData("article"), 'Loading...')

    return {
      components: { Article },
      data() {
        return {
          art: {
            link: "test",
            title: dt,
            desc: "test",
          }
        }
      },
      template: `<Article v-bind="art" />`
    }
  })

[husayt]

@Hypnosphi Thank you for the direction. I have been playing with this since then, but seems it is not ready yet.

This is what I ended up with:

import { useEffect, useState } from "@storybook/client-api"
const dt, setDt

storiesOf("Article", module)
  .add("test", () => {
   ;[dt, setDt] = useState('Loading...')
   console.log("rendering started", dlist)
   useEffect(() => {
      getDynData("article").then(result => {
          setDt(result)
      })
    },[])

    return {
      components: { Article },
      data() {
        return {
          art: {
            link: "test",
            title: dt,
            desc: "test",
          }
        }
      },
      template: `<Article v-bind="art" />`
    }
  })

Let me go through issues I faced.

1. Seems this has been updated and I should use client-api for import
   import { useEffect, useState } from "@storybook/client-api"
2. Above example was causing endless loop as setData would cause rerender and on each one dt would be undefined. So I had to
   a. define dt, setDt outside story, so that value would be retained.
   b . Also, instead of mounted logic which you put to tackle infinite calling, I am passing [] as second param to useEffect and that ensures its called only once.

But still after all this data doesn't get updated on the Vue component. Vue is not getting updates to dt. Seems, there needs to be some kind of bridge to pass reactive data from dt to Vue.

I see console.log("rendering started", dlist) is being called twice, first time with ..Loading and second time with the value from api, but component is only rendered once thus only mounted once. Updates to dt don't make any difference to the component.

So seems like these are Vue related issues. Most probably we are jumping ahead of the line with that, as nowhere it is mentioned that this is expected to work with Vue.

BUt I can see how extremely powerful this can be

[Hypnosphi]

1. Sure, sorry
2. Just b. should be enough. You still need that mounted logic to avoid errors when you switch story before the data arrives

I'll update my previous comment with your edits.

As for issues with updates, I'm no expert in Vue, but maybe you should use props instead of data? At least, that's what's used for dynamic values in Knobs example