storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.02k stars 9.23k forks source link

preview > decorators causes: "Rendered more hooks than during the previous render." #15223

Closed pixelass closed 1 year ago

pixelass commented 3 years ago

Describe the bug When adding a decorator to .storybook/preview.js an error occurs: "Rendered more hooks than during the previous render."

Error
Rendered more hooks than during the previous render.
Call Stack
 useHook
  vendors~main.iframe.bundle.js:11148:13
 useMemoLike
  vendors~main.iframe.bundle.js:11175:18
 useMemo
  vendors~main.iframe.bundle.js:11187:10
 withBackground
  vendors~main.iframe.bundle.js:3858:106
 undefined
  vendors~main.iframe.bundle.js:11056:21
 undefined
  vendors~main.iframe.bundle.js:12992:12
 undefined
  vendors~main.iframe.bundle.js:12999:14
 renderWithHooks
  vendors~main.iframe.bundle.js:249128:18
 mountIndeterminateComponent
  vendors~main.iframe.bundle.js:251954:13
 beginWork
  vendors~main.iframe.bundle.js:253192:16
Screen Shot 2021-06-13 at 15 10 48

To Reproduce Repro: TBD

Add the following code to your .storybook/preview.js

export const decorators = [Story => <Story />];

System

  System:
    OS: macOS 11.4
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  Binaries:
    Node: 14.16.0 - ~/.nvm/versions/node/v14.16.0/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.11 - ~/.nvm/versions/node/v14.16.0/bin/npm
  Browsers:
    Chrome: 91.0.4472.101
    Firefox: 87.0
    Safari: 14.1.1
  npmPackages:
    @storybook/addon-actions: ^6.2.9 => 6.2.9 
    @storybook/addon-essentials: ^6.2.9 => 6.2.9 
    @storybook/addon-links: ^6.2.9 => 6.2.9 
    @storybook/react: ^6.2.9 => 6.2.9 
strothj commented 3 years ago

Seems to happen for me when React Refresh is enabled.

shilman commented 3 years ago

Can you please create a reproduction by running npx sb@next repro, following the instructions, and linking it in your issue description? We prioritize issues with reproductions over those without. Thank you! 🙏

benbender commented 3 years ago

@shilman It should be the same problem as in storybookjs/addon-knobs#2 - reacts StrictMode. Had the same issue with storybook-addon-next-router.

@strothj @pixelass Could you check and confirm if you had reactOptions.strictMode = true in main.js?

pixelass commented 3 years ago

I definitely did not set reactOptions.strictMode = true Sadly I haven't had time to create a reproduction project yet.

thebergamo commented 3 years ago

I've also the similar issue and only way I could get rid of it, was removing my exported decorators in preview.js file. Without it it started to work properly.

pascalduez commented 3 years ago

I just enabled reactOptions.strictMode and hitting this error as well.

Seems to come from a combination of reactOptions.strictMode, a preview.js decorator, and addons. I can make it go away by either removing the decorator, or disabling @storybook/addon-essentials.

Will try to make repro.

seppzero commented 2 years ago

We just also run in that issue.

@shilman here is a reproduction-repo based on sb@next init https://github.com/seppzero/sb-decorator-issue. I hope that helps.

redbugz commented 2 years ago

We just ran into this issue upgrading from storybook 6.3 to 6.4. One thing I noticed when stepping through the debugger:

In the jsxDecorator:

export var jsxDecorator = function jsxDecorator(storyFn, context) {
  var _context$parameters$d2, _context$parameters$d3;

  var channel = addons.getChannel();
  var skip = skipJsxRender(context);
  var story = storyFn();
  var jsx = '';
  useEffect(function () {
    if (!skip) channel.emit(SNIPPET_RENDERED, (context || {}).id, jsx);
  }); // We only need to render JSX if the source block is actually going to
  // consume it. Otherwise it's just slowing us down.

  if (skip) {
    return story;
  }

Our story triggers Suspense due to a third-party library we use, so StoryFn() throws a promise the first time, which I believe will make that useEffect following not run, but then 2nd render once the suspense is resolved, then storyFn will run normally and then the useEffect runs, which then triggers the too many hooks error. I believe this particular case could be solved by moving the storyFn call below the useEffect.

However, looking at the reproduction repo that was made, it seems to be failing in withOutline instead, and I don't see any problematic code there that would conditionally execute a hook, so it seems like there might be something more fundamental going on?

redbugz commented 2 years ago

After more investigation, I think there are 2 separate issues here. I will open a new issue for the issue with jsxDecorator and Suspense, it seems unrelated to the original issue with strictMode and preview decorators, as I can reproduce my issue regardless of the strictMode or decorators status.

For the original issue here, I added some logging to the addons/dist/esm/hooks.js when running the repro. I don't completely understand the entire flow with the hooks, but here is what I observed with the original issue here. I created a new repro with the plain CRA template, then just add the decorator to preview.js and enable strict mode and then the hooks error shows up on all the stories.

With no strict mode and no preview decorator, the useHook gets called several times in MOUNT, but no UPDATE and it works Same with a decorator but no strict mode, it just calls several times in MOUNT and works. If you enable strictMode without a decorator, you get the MOUNT calls, but then lots of UPDATE but everything still works. However, as soon you have both strict mode and a decorator, then useHook is never called with MOUNT, but only a few UPDATE and they fail because no hooks were ever added in the MOUNT. I have no idea why strict mode plus a decorator would cause the MOUNT phase to be skipped entirely, but that's what seems to happen. Hopefully that helps someone that understands this better know what is happening.

penx commented 2 years ago

I encountered this error after enabling strict mode and fast refresh, and was able to resolve it in my decorator function by changing <Story /> to {Story()}

i.e.

// preview.tsx ❌ errors
import {DecoratorFn} from '@storybook/react';

export const decorators: DecoratorFn[] = [
  Story => {
     return (
      <ThemeProvider>
        <Story />
      </ThemeProvider>
    );
  },
];
// preview.tsx ✅ works
import {DecoratorFn} from '@storybook/react';

export const decorators: DecoratorFn[] = [
  story => {
    return (
      <ThemeProvider>
        {story()}
      </ThemeProvider>
    );
  },
];
redbugz commented 1 year ago

We just ran into this again, only this time it was caused by running storybook through Cypress tests. We have a decorator for our stories that need authentication to prompt to authenticate, and that decorator works fine in storybook alone, but as soon as we try to hit those stories with Cypress tests, then we get the hooks error and those stories fail. So that's a new wrinkle. I will continue to investigate and see if I can figure out why running inside the Cypress runner iframe causes the rendering and hooks to behave differently when just running normally.

Switching our decorator from <Story /> to {Story()} did not help in this case

seonghyeonkimm commented 1 year ago

I am still facing the same issue. is there another workaround to try? I just commented out @storybook/addon-essentials. it works however I want to use essentials addon too.

YuvalZiegler commented 1 year ago

I have the same issue. Moving var story = storyFn(); after useEffect in jsxDecorator.js as suggested here fixed it for me.

Another option was configuring @storybook/addon-essentials:

  name: '@storybook/addon-essentials',
  options: {
    // these three do not cause issues
    actions: true,
    backgrounds: true,
    viewport: true,
    // these two caused the "Rendered more hooks than during the previous render" error
    controls: false,
    docs: false,
  }

but that's not an option for me.

agriffis commented 1 year ago

FWIW I'm on v7, I have a stack of decorators and I had a lot of issues, especially Storybook's useArgs blowing up after the initial render. I converted all my decorators from:

Story => <Whatever><Story /></Whatever>

to

(storyFn, context) => <Whatever>{storyFn(context)}</Whatever>

and it seems to have solved the problems for me.

redbugz commented 1 year ago

I believe I have discovered another detail in the root cause. I discovered that our decorator conditionally renders the story function, as it waits for the user to be logged in before rendering the story. So the first few times it renders with a message that the story requires auth and a sign in button, then once they are signed in, it renders the story. This is what seems to cause the "Rendered more hooks". If I always render the story function it works fine, if I never render the story function it works fine, but if I render a time or two without the story function, then render the story function, the code in storybook that handles the hook context doesn't like that, because suddenly when the story function renders a lot more hooks are getting called than when the sign in button is shown. If I always render the story, just render it in a hidden div until they are signed in, it works fine. It must have something to do with the fact that our cypress tests sign the user in, but it must try to render the story just before the sign in is complete, so it renders the sign in button once or twice, then sign in completes, and now it renders the entire story, and the error occurs. I am going to try to either gate the sign in better, or make the decorator better at handling this. As a fallback, I can always render the story in a hidden div until it's complete.

alex-knyazev commented 1 year ago

In my example, I want to render Story component with some delay:

export const decorators = [
    // Site decorator
    ( Story, { globals: { site } } ) => {
        document.documentElement.id = 'next-frontend-html';
        document.body.id = 'next-frontend-body';

        const configsInjected = useGlobalJsonConfigs( site );

        if ( ! configsInjected ) {
            return 'loading';
        }

        return (
            <div className={ style.wrapper }>
                <link
                    rel="stylesheet"
                    type="text/css"
                    href={ `/app-configs/${ site }/global/design.css` }
                />
                <GlobalFonts siteName={ site } />
                <Story />
            </div>
        );
    },
];

So, don't I have a way to render a Story conditionally?

jcfilben commented 1 year ago

@alex-knyazev In my setup I also wanted to render a story conditionally. I ended up doing the following as a workaround:

export const decorators = [
  ( Story, context ) => {
    ...
    if ( contentIsLoading ) {
      return (
        <p>loading</p>
        <div hidden>
          <Story />
        </div>
      );
    }

    return (
      <Story />
    );
  },
];
ccpu commented 1 year ago

This is how i do it:

export const MyDecorator  = (story: () => unknown) => {
  const [loaded, setLoaded] = React.useState<boolean>();

  React.useEffect(() => {
    setLoaded(true);
  }, []);

  const Story = (
    <Wrapper>
        {story()}
    </Wrapper>
  );

  if (!loaded) return <div>loading ...</div>;

  return Story;
};
marybeshaw commented 1 year ago

I have reproduced this issue today. https://github.com/marybeshaw/storybook-issue-15223

@seppzero reproduced this issue a year ago: https://github.com/seppzero/sb-decorator-issue.

@shilman is there anything else you need to take off the "needs reproduction" tag?

marybeshaw commented 1 year ago

In my repro, the stackblitz link is wrong; it's the original version of preview.js, not the one I changed.

soullivaneuh commented 1 year ago

We got the same issue. According to our internal git bisect investigation, the crash was introduced by the update of Storybook from 7.0.2 to 7.0.4.

Also, we precised we have the issue only by going to the dedicated "Docs" section of our stories. Classic story rendering works as expected.

Our decorator is configured like so:

// .storybook/preview.ts
export const decorators: DecoratorFn[] = [
  StoryDecorator,
];

Our decorator:

import React, {
  FC,
  ReactElement,
  Suspense,
  useEffect,
} from 'react';
import {
  DecoratorFn,
} from '@storybook/react';
import {
  themes,
} from '@storybook/theming';
import {
  useDarkMode,
} from 'storybook-dark-mode';
import {
  memoryStore,
  useLocaleState,
} from 'react-admin';
import AppContext from '../../src/AppContext';
import {
  fakeDataProvider,
} from '../../src/providers';

type AppDecoratorProps = {
  locale: string;
  children: ReactElement;
}

const AppDecorator: FC<AppDecoratorProps> = ({
  locale,
  children,
}) => {
  const [_, setLocale] = useLocaleState();

  useEffect(() => {
    setLocale(locale);
  }, [locale])

  return children
}

export const StoryDecorator: DecoratorFn = (Story: any, {
  globals: {
    locale,
  },
  parameters,
}) => {
  const dark = useDarkMode();

  // @see https://github.com/hipstersmoothie/storybook-dark-mode/issues/235
  useEffect(() => {
    const backgroundColor = dark ? themes.dark.appBg : themes.light.appBg;
    document.body.style.backgroundColor = backgroundColor || 'inherit';
  }, [dark]);

  return (
    <React.StrictMode>
      <Suspense fallback="loading...">
        <AppContext
          memoryRouter={parameters.memoryRouter}
          darkMode={dark}
          dataProvider={fakeDataProvider()}
          store={memoryStore()}
        >
          <AppDecorator locale={locale}>
            <Story />
          </AppDecorator>
        </AppContext>
      </Suspense>
    </React.StrictMode>
  );
};

export default null;
marybeshaw commented 1 year ago

My team has reproduced this issue with Storybook 6.3 and also Storybook 7, so maybe there are different causes?

redbugz commented 1 year ago

I'm trying to give it a stab at fixing this issue as it is blocking a lot of our storybook work. After putting log messages where I think it's useful, I think I have some kind of rudimentary understanding of what is going on, but the lifecycle around this is a bit involved so I'm still not clear about everything that is going on. It appears to me that Storybook intercepts all of our hooks, and tries to add some of it's own management in between our hooks and React, and that code seems to be making some assumptions that are not true. From what I understand, as each decorator and story is rendered, it goes through a MOUNT state where it records/saves off all the hooks that it used, and then when it re-renders, it goes through an UPDATE state where that hook state is restored. What I'm seeing in the logs, is that when we run into this issue, the children of a decorator that are conditionally rendered go straight to UPDATE without ever going through MOUNT, and since it didn't get a chance to save off the hook state, it tells you that you rendered more hooks than last time. I believe the problem is here: https://github.com/storybookjs/storybook/blob/v7.1.0-alpha.11/code/lib/preview-api/src/modules/addons/hooks.ts#L194 where when they call applyHooks and applyDecorators they set prevMountedDecorators to the entire list of all registered decorators plus the story fn, assuming that they all render every time, but if any children are conditionally rendered that assumption is false, or part of the decorator chain will render the first time, and then only later will the remaining set of decorators execute. So the first render, only some of the decorators will go through MOUNT, and on subsequent renders it assumes they've all gone through MOUNT when they haven't. I crafted a unit test in my fork with a decorator that conditionally renders it's children, and it does show the "Rendered more hooks" error in the unit test, so that is progress, but switching that line of code to

      hooks.prevMountedDecorators = new Set();

does not fix the unit test like I thought it would, but it might be because of how the unit tests work. They have this special run function to simulate running the decorators multiple times, but when I log out what is happening it doesn't follow the same behavior that a story logs when it renders in the repros, so I'm thinking the unit tests run slightly differently than the actual code, and I don't understand this lifecycle well enough to figure out how. I've started a thread in the Storybook discord to see if anyone can help me. When running the storybook repo locally in development mode, they have a way to link in your changes into the repro directories people have created to validate the fix, but unfortunately, even though the repros correctly show the errors, as soon as link them into the dev branch of storybook with my changes, they get new "Invalid hook" errors, which typically means multiple copies of React running in the same code, which often happens when linking, so I don't know how to fix that in such a complex repo as storybook either.

redbugz commented 1 year ago

Looks like I'm somewhat mistaken. The internal decorators use custom look-alike hooks, so Storybook is not intercepting the hooks from user-provided decorators and stories that use React hooks, but preview-api has it's own custom React-like hooks that the built-in decorators use. The problem appears to be with how the jsxDecorator works. Without our custom decorator the conditionally renders the story, these are the phases:

image

No errors, but also no UPDATE, only MOUNT. However, once our custom decorator is added, the biggest change is now the story has to go through the update phase, and the jsxDecorator gets out of sync. It never gets MOUNT called, but it gets UPDATE called twice at the end, and now we get the "Rendered more hooks" error:

image

jsxDecorator uses 1 hook, useEffect, but since it never got MOUNT and had the state saved off, when it gets to UPDATE it has no hooks registered and throws the error. It doesn't seem to matter that it got called twice at the end, the first call throws the error, the 2nd one also throws the error for good measure, but it appears having the conditional decorator interferes with jsxDecorator going through MOUNT, and causes it UPDATE to be a bit async compared to the others perhaps, as you will notice there are 2 sets of updates, and jsxDecorator doesn't participate in the first round, so I think it must get queued somehow, and then both of them drop at the very end.

redbugz commented 1 year ago

I missed one spot where it was setting prevMountedDecorators, once I fixed that one too, now it works for both repros, so I think we have a working solution. See the linked PR

tmeasday commented 1 year ago

For those wondering why this issue may have started in 7.0.3 -- we moved the JSX decorator to the end of the list in https://github.com/storybookjs/storybook/pull/21907

So I suppose if there was a second "conditional" decorator that previously came after the JSX decorator, it now comes before, leading to this bug. But the issue can happen if you have conditional decorators no matter what, I suppose, it just depends on exactly what decorators you have.

bel0v commented 1 year ago

For us the issue started on storybook 7.0.2, when we upgraded @storybook/builder-vite and @storybook/react-vite from 7.0.2 to 7.0.7. Reverting these two back solved the issue.

mcrider commented 1 year ago

Sorry for the impatience but the PR (https://github.com/storybookjs/storybook/pull/22336) seems to need just a nudge to get across the finish line, perhaps a core contributor could cherry-pick the test over if @redbugz isn't available? Getting this fix merged in would be a huge help for me.

tmeasday commented 1 year ago

Thanks for the reminder @mcrider. I can't push to @redbugz's branch so I'll just need to make a new PR from my branch I guess.

KorySchneider commented 1 year ago

I am getting this error on stories that have decorators (not conditional). Changing the syntax from <Story /> to {Story()} in the decorators seems to be a viable workaround for now.

Using Storybook 7.0.18 with Vite and React.

KorySchneider commented 1 year ago

~I am still getting this error on Storybook 7.0.20~ My bad, looks like the fix is going to be released in 7.1.0

isc30 commented 1 year ago

Hey, the workarounds listed here don't work. I'm hitting this with latest SB version (7.0.23), when is 7.1.0 going to be released?

tmeasday commented 1 year ago

@shilman should we patch https://github.com/storybookjs/storybook/pull/22336 back to 7.0?

NoHop3 commented 12 months ago

Seeing the same problem now on 7.4.4 -> Had no problems on 7.4.2 before updating

kasir-barati commented 9 months ago

I had this issue because I was creating a new React component to satisfy my eslink react hook rules like this:

import type { Meta, StoryObj } from "@storybook/react"
import { CName, CNameProps } from './cname'

export default { component: CName } satisfies Meta<typeof CName>
type Story = StoryObj<typeof CName>

function Wrapper() {
  const [args, updateArgs] = useArgs<CNameProps>()
  // ...
  return <CName {/* ... */} >
}

export const storyName: Story = {
  // ...
  render: (args) => <Wrapper {...args} />
  // ...
}

But then I just changed it to a normal storybook style and add // eslint-disable-next-line react-hooks/rules-of-hooks on top of useArgs:

import type { Meta, StoryObj } from "@storybook/react"
import { CName, CNameProps } from './cname'

export default { component: CName } satisfies Meta<typeof CName>
type Story = StoryObj<typeof CName>

export const storyName: Story = {
  // ...
  render() {
    // eslint-disable-next-line react-hooks/rules-of-hooks
    const [args, updateArgs] = useArgs<CNameProps>()
    // ...
    return <CName {/* ... */} >
  }
  // ...
}

Hopefully this can help someone 😄

rohitDalalStrique commented 4 months ago

seeing the same problem on 8.0.10. Also changing <Story /> to story() didn't work for me.