[Bug]: React component seems to be mounted twice when a story render function uses the args parameter

mirobossert commented 1 year ago

Describe the bug

When using the args parameter in the render function of a Storybook story, the React component appears to be mounted twice. This behavior is causing issues, especially when using a custom hook directly in the story.

To Reproduce

Go to the provided StackBlitz link.
Open the the Button stories Working and Not Working and observe the browser console to see the different behaviour of the two stories.

Working: The hook in the render function works as expected, it logs a message to the console on each second click on the button
Not Working: The component seems to get mounted twice. One instance seems to behave correctly the other one does not update it's state. As a result it logs once on each click and twice on each second click. The only difference between the two stories seems to be the args parameter gets passed into the render function.

https://stackblitz.com/edit/github-56csk3?file=stories%2FButton.stories.tsx

System

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  Binaries:
    Node: 16.20.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.4.2 - /usr/local/bin/npm
  npmPackages:
    @storybook/addon-essentials: ^7.4.0-alpha.0 => 7.4.0-alpha.0 
    @storybook/addon-interactions: ^7.4.0-alpha.0 => 7.4.0-alpha.0 
    @storybook/addon-links: ^7.4.0-alpha.0 => 7.4.0-alpha.0 
    @storybook/addon-onboarding: ^1.0.8 => 1.0.8 
    @storybook/blocks: ^7.4.0-alpha.0 => 7.4.0-alpha.0 
    @storybook/nextjs: ^7.4.0-alpha.0 => 7.4.0-alpha.0 
    @storybook/react: ^7.4.0-alpha.0 => 7.4.0-alpha.0 
    @storybook/testing-library: ^0.2.0 => 0.2.0

Additional context

I understand that the observed behavior might be intentional or could potentially be influenced by the implementation of the custom hook. While I've tried to isolate the issue to the best of my ability, it's possible that there are factors I'm not aware of that could contribute to this behaviour.

Any assistance, guidance, or ideas on how to approach this issue would be highly valued. Thank you in advance for your help.

cbrianball commented 6 months ago

I have run into this issue as well. It looks like the function arguments don't even need to be used, by the fact that they are defined on the function is enough to cause it to render twice.

I have found a workaround, if you put all of the function arguments into a single rest argument, the story appears to only render once. It's not pretty, but it works for me.

This causes the story to render twice:

export const MyStory = (args) => {...};

But this will cause the story to only render once (as expected):

export const MyStory = (...[args]) => { ... }

Fi1osof commented 2 months ago

Confirm that args causes mounting twice. And each time args - same object.

let argsMemorized: object | undefined = undefined

const CheckedIcon: React.FC = (args) => {
  if (argsMemorized === undefined) {
    console.log("no argsMemorized")
    argsMemorized = args
  }

  useEffect(() => {
    console.log("CheckedIcon mounted args", args)
    console.log(
      "CheckedIcon mounted args === argsMemorized",
      args === argsMemorized
    )

    return () => {
      console.log("CheckedIcon unmounted")
    }
  }, [args])

  return <Renderer src={checkedIcon.src} />
}

Output:

no argsMemorized
(index):75 Renderer mounted
(index):107 CheckedIcon mounted args {}
(index):108 CheckedIcon mounted args === argsMemorized true
(index):107 CheckedIcon mounted args {}
(index):108 CheckedIcon mounted args === argsMemorized true

And confirm, this workaround works.

ndelangen commented 1 month ago

I plan to take a look at this.

ndelangen commented 1 month ago

@mirobossert I suspect it's react in strict-mode that's acting differently based on wether args is used in the custom render function.

JReinhold commented 4 weeks ago

I've investigated this and concluded that the source of this bug is excludeDecorators.

I suspect it's react in strict-mode that's acting differently based on wether args is used in the custom render function.

this isn't the case @ndelangen, we're not rendering in strict mode, that is something the user explicitly adds to the story when needed.

The double rendering happens because of this logic: https://github.com/storybookjs/storybook/blob/57885602a11e48ec8abe41ab5380c6291db68bf0/code/renderers/react/src/docs/jsxDecorator.tsx#L256-L259

Basically, when we want to exclude decorators from the source generation (for docs), we first execute the originalStoryFn() to get the undecorated JSX output. This is an execution of the story-function separately from the story rendering itself, which is why it "renders" twice.

(Interestingly this doesn't just impact the initial rendering - as in the reproduction above, when you click the button to flip the state, that also logs twice, even though only one is rendered. My guess is that executing the same instance of a React component multiple times isn't really compatible with React.)

The reason this seems related to args, is because when a story has args it gets the __isArgsStory parameter set (via static analysis), which controls if rendering-for-source-output should be skipped or not: https://github.com/storybookjs/storybook/blob/57885602a11e48ec8abe41ab5380c6291db68bf0/code/renderers/react/src/docs/jsxDecorator.tsx#L197-L209

I've confirmed this by setting parameters.docs.source.code = 'something static' which also skips source-rendering as you can see above, and that too "fixes" the issue.

Finally, setting parameters.docs.source.excludeDecorators = false also fixes the issue.

The logic that executes originalStoryFn() was introduced in Storybook 6.3 via https://github.com/storybookjs/storybook/pull/14652. However, excludeDecorators have historically been false by default, but it became true by default for Next.js projects in Storybook 7.0.0-beta-48 via https://github.com/storybookjs/storybook/pull/21029

@shilman @tmeasday do you have any good ideas for how to fix this, beyond completely reworking our source-generation logic?

tmeasday commented 4 weeks ago

Great work figuring that out @JReinhold! I want to say I remember something like this happening once before and us reordering decorators in order to not have to enable excludeDecorators for React. Does that ring any bells @shilman?

JReinhold commented 4 weeks ago

Great work figuring that out @JReinhold! I want to say I remember something like this happening once before and us reordering decorators in order to not have to enable excludeDecorators for React. Does that ring any bells @shilman?

@tmeasday here's the paper trail I've found:

reorder decorators in https://github.com/storybookjs/storybook/pull/21182 by @valentinpalkovic
default to exclude decorators everywhere in https://github.com/storybookjs/storybook/pull/21722 by @tmeasday
open issue https://github.com/storybookjs/storybook/issues/21900 about that change with "It seems like decorators are no longer used in some situations, depending on whether arguments are provided to a custom render function" (interesting, huh?)
revert the default exclusion of decorators in https://github.com/storybookjs/storybook/pull/21902

I find this specific change by @tmeasday interesting:

https://github.com/storybookjs/storybook/pull/21722/files#diff-fa2456068b391a5bc726772d8fffba63030f53d2e8b486c21ba354b584edd3cdL32

Where we don't execute story anymore but just pass as-is - could we do the same with originalStoryFn or is that a totally different function signature?

tmeasday commented 4 weeks ago

@JReinhold I think that change doesn't quite do that, I think it actually always calls storyFn() in those two renderers, I think this is just a bug fix, previously those two renderers were broken when you excluded decorators (that's why @shilman didn't revert that part).

Thank you for the leg work so I think the history here is:

We added some decorators and reordered in NextJS so some get between the source decorator and the story
This meant the source block was useless in NextJS as it rendered the decorator
We turned on exclude decorators everywhere by default to avoid this, but this had side-effects like this issue.
We reverted that and just did it for NextJS.

I wonder if ultimately we need a different approach for excludeDecorators maybe? I'm not sure. I think we are stuck between a rock and a hard place here for NextJS with the current approach.

A hacky solution we had considered was some mechanism to "force" the source decorator to be the last (or would you call it first) one, somehow.

JReinhold commented 3 weeks ago

A hacky solution we had considered was some mechanism to "force" the source decorator to be the last (or would you call it first) one, somehow.

I can see that working, but do we then put it as the innermost decorator to only show the pure story, or should it still encapsulate the user-defined decorators in the project, meta and story-level? I'm unsure what the desired behavior is.

Perhaps we acknowledge that if you put something in a decorator, you don't want it in the source view - if you want a wrapper displayed in the source view, it must go in render?

tmeasday commented 3 weeks ago

I think that sounds right

storybookjs / storybook