Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
pixelass opened this issue Jun 13, 2021 · 36 comments · Fixed by #22336
Closed

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

pixelass opened this issue Jun 13, 2021 · 36 comments · Fixed by #22336

Comments

@pixelass
Copy link

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 
@pixelass pixelass changed the title decorators causes "Rendered more hooks than during the previous render." preview > decorators causes: "Rendered more hooks than during the previous render." Jun 13, 2021
@strothj
Copy link

strothj commented Jun 27, 2021

Seems to happen for me when React Refresh is enabled.

@shilman
Copy link
Member

shilman commented Jun 27, 2021

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
Copy link
Contributor

benbender commented Jul 16, 2021

@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
Copy link
Author

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

@thebergamo
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

redbugz commented Mar 30, 2022

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
Copy link
Contributor

redbugz commented Mar 31, 2022

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
Copy link
Contributor

penx commented Apr 8, 2022

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
Copy link
Contributor

redbugz commented Nov 23, 2022

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
Copy link

seonghyeonkimm commented Feb 17, 2023

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
Copy link

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
Copy link
Contributor

agriffis commented Mar 29, 2023

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
Copy link
Contributor

redbugz commented Apr 7, 2023

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
Copy link

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
Copy link

@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
Copy link
Contributor

ccpu commented Apr 16, 2023

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
Copy link

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
Copy link

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

@soullivaneuh
Copy link

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
Copy link

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

@redbugz
Copy link
Contributor

redbugz commented May 1, 2023

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
Copy link
Contributor

redbugz commented May 1, 2023

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 added a commit to redbugz/storybook that referenced this issue May 1, 2023
Fixes storybookjs#15223
Custom preview-api hooks assumed that all decorators were always rendered
however if a custom decorator conditionally rendered it's children, then
not all decorators would get rendered, especially jsxDecorator.
This would result in the error
"Rendered more hooks than during the previous render."
This removes the assumption that all decorators render every time
and relies on each decorator to register itself during MOUNT phase
which is handled when each decorator goes through `hookify`
@redbugz
Copy link
Contributor

redbugz commented May 1, 2023

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
Copy link
Member

tmeasday commented May 3, 2023

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 #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
Copy link

bel0v commented May 12, 2023

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
Copy link

mcrider commented May 25, 2023

Sorry for the impatience but the PR (#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
Copy link
Member

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.

tmeasday pushed a commit to redbugz/storybook that referenced this issue Jun 1, 2023
Fixes storybookjs#15223
Custom preview-api hooks assumed that all decorators were always rendered
however if a custom decorator conditionally rendered it's children, then
not all decorators would get rendered, especially jsxDecorator.
This would result in the error
"Rendered more hooks than during the previous render."
This removes the assumption that all decorators render every time
and relies on each decorator to register itself during MOUNT phase
which is handled when each decorator goes through `hookify`
@KorySchneider
Copy link
Contributor

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
Copy link
Contributor

KorySchneider commented Jun 8, 2023

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
Copy link

isc30 commented Jun 22, 2023

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
Copy link
Member

@shilman should we patch #22336 back to 7.0?

@NoHop3
Copy link

NoHop3 commented Sep 22, 2023

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

@kasir-barati
Copy link

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.