Storyshots: update render w/ React.createElement

[tmikeschu]

Issue: #8177

What I did

Wrapped the story.render() calls with React.createElement for storyshots.

This allows you to use hooks in CSF without wrapping components in a thunk-like function.

How to test

• Is this testable with Jest or Chromatic screenshots?
  Jest

• Does this need a new example in the kitchen sink apps?
  Nope

• Does this need an update to the documentation?
  Nope

If your answer is yes to any of these, please make sure to include it in your PR.

[vercel]

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/p7bzv1r46
🌍 Preview: https://monorepo-git-fork-tmikeschu-ms-storyshots-render-update.storybook.now.sh

[tmikeschu]

Hmmm all my tests pass locally with only these changes and only running the yarn bootstrap setup.

[shilman]

@tmikeschu i think at this point you just need to update snapshots

yarn test --core --update

[tmikeschu]

@shilman I had the same thought, but I don't get a diff/updates from that. I'm perplexed!

[shilman]

Snapshots are updating in a kind of weird way. I think this qualifies as a breaking change, but if it seems correct we can work this into our next release, which will be 6.0.

@tmikeschu @Hypnosphi please lmk what you think!

[tmikeschu]

Interesting. Is it possible the discrepancy is yarn version? (it's not)

I'm not sure what the best solution is, but let me know how I can help!

@shilman where is that diff from? When I look on Circle CI, I just see a new <storyFn> wrapping node. <button> appears to be nested within <Button>, not replacing it.

[shilman]

@tmikeschu that's a screenshot from github desktop. there were a bunch of diffs that were all pretty similar, i just chose one at random. I suspect that this change is fine and we'll just need to wait until the next release cycle to merge it. I'm pretty sure @Hypnosphi will have an opinion, so let's see what he has to say.

[Hypnosphi]

I think we can't use shallow rendering with this approach, because now we snapshot the story return value, not the one of component

[Hypnosphi]

I just see a new <storyFn> wrapping node. <button> appears to be nested within <Button>

That's the case for default renderer and enzyme's mount

replacing it.

And that's the case for shallow renderers: react-test-renderer/shallow which is default in renderShallowTree, and enzyme's shallow. In this case we lose the information about actual component output (@shilman note the removed style prop), which is a large regression. Unfortunately, I don't have a clear solution in mind.

UPD: one option is to use two-pass rendering: use the output of first shallow rendor as input for the second one

// in renderShallowTree.ts 

const createTwoPassRenderer = () => {
  const shallowRenderer = shallow.createRenderer();
  return {
    render(storyElement) {
      const storyOutput = shallowRenderer.render(storyElement).getRenderOutput();
      return shallowRenderer.render(storyOutput);
    }
  }
}

function getRenderedTree(...) {
  ...
  const shallowRenderer = renderer || createTwoPassRenderer();
  ...
}

If users provide their own shallow renderer, they will have to implement something similar on their side

[Hypnosphi]

I don't get a diff/updates

@tmikeschu this can happen if your dist directories are outdated. You should either run yarn bootstrap --core once again, or have yarn dev running in background to transpile all your changes on the fly

[shilman]

@Hypnosphi thanks for all the investigation on this. nothing's ever easy, is it? i'm not sure what a good solution is here. current solution with workaround is non-ideal, but it sure is simpler!

[Hypnosphi]

Just to be clear. Current solution makes shallow storyshots test a comletely different thing, comparing to what they tested before. Given a component like that:

export const MyComponent = ({someProp}) => (
  <SomeOtherComponent
    someImportantProp={someLogic(someProp)}
  />
)

and a story like that:

export const MyStory = () => <MyComponent someProp={someValue}  />

Before the change, the shallow storyshot of the story would output something like that:

<SomeOtherComponent
  someImportantProp={valueDependingOnComponentLogic}
/>

And after the change, it's

<MyComponent someProp={someValue}  />

So now, instead of testing your component, you only test your story. But it's the component what you actually use in your product, and therefore want to test

[shilman]

@Hypnosphi thanks for clarifying. then maybe we should do this change only for deep rendering and not for shallow rendering?

[Hypnosphi]

Maybe so. By my suggestion with two-pass rendering may work as well

[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 30 days. Thanks!

[stale]

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!