React: Fix decorators to conditionally render children

[redbugz]

Fixes #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

Closes #15223

What I did

Previously hooks.ts would always set hooks.prevMountedDecorators to the entire list of registered decorators, which assumed they would all render and mount. This changes that behavior to set that to an empty Set in init and only update that in applyDecorators if it's nullish.
As each decorator goes through the MOUNT stage, then it will add itself to the prevMountedDecorators set so it will accurately only move to the UPDATE stage if MOUNT has already happened.

There are a lot of comments at the bottom of #15223 detailing the process I went through to discover this and what I learned and how I came to this conclusion and fix.

How to test

See repro at https://github.com/marybeshaw/storybook-issue-15223

Started storybook with yarn storybook then any story will trigger the error, I used Button mostly.

I couldn't get the instructions for yarn linking to work locally, so I checked out that repo, updated it to yarn 3 and storybook 7.1.0-alpha.0, then manuallly made the changes to node_modules/@storybook/preview/dist/runtime.js to verify the fix. Also used the unit test to confirm the desired behavior. Without the fix, the unit test fails with the "Rendered more hooks" error.

Also verified against repro: https://github.com/seppzero/sb-decorator-issue

Once again, couldn't get linking to work, so I checked out the repo, built it using the 6.5 version it had, ran storybook, the Button story fails with the "Rendered more hooks" error, manually patched node_modules/@storybook/addons/dist/esm/hooks.js , restarted storybook, now the Button story works fine

I don't believe any documentation needs to be updated for this.

Checklist

• Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
• Make sure to add/update documentation regarding your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Maintainers

• If this PR should be tested against many or all sandboxes,
  make sure to add the ci:merged or ci:daily GH label to it.
• Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

[tmeasday]

I think this seems reasonable given my (limited) understanding of the motivation of the hook restrictions. Similar to the way that React can scope hooks to a single component, we can scope hooks to a single decorator, correct?

@redbugz -- I think it would be good to add a "template story" to preview-api and a test (either e2e or just an interaction test on the story) that demonstrates the error no longer occurs.

[redbugz]

@redbugz -- I think it would be good to add a "template story" to preview-api and a test (either e2e or just an interaction test on the story) that demonstrates the error no longer occurs.

Sure I can give that a try. I'm still learning this codebase so I'm not sure where that would go. Poking around, would it be adding a story to https://github.com/storybookjs/storybook/blob/next/code/renderers/react/template/stories/decorators.stories.tsx or https://github.com/storybookjs/storybook/blob/next/code/renderers/react/template/stories/hooks.stories.tsx or a new file in that location or a different location like adding a templates/stories dir inside of https://github.com/storybookjs/storybook/tree/next/code/lib/preview-api?

Would an e2e test go here https://github.com/storybookjs/storybook/blob/next/code/e2e-tests/preview-web.spec.ts or somewhere else?

[tmeasday]

Hey @redbugz sorry if it wasn't clear but I posted some advice in discord, repeating here for clarity:

if you can run the sandbox, then next take a look at this file:

storybook/code/lib/store/template/stories/args.stories.ts

Lines 66 to 82 in e22ac0d

	export const Events = {
	args: {
	test: 'initial',
	},
	parameters: { argNames: ['test'] },
	play: async ({ canvasElement, id }: PlayFunctionContext<any>) => {
	const channel = globalThis.__STORYBOOK_ADDONS_CHANNEL__;
	await channel.emit(RESET_STORY_ARGS, { storyId: id });
	await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve));
	await within(canvasElement).findByText(/initial/);
	await channel.emit(UPDATE_STORY_ARGS, { storyId: id, updatedArgs: { test: 'updated' } });
	await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve));
	await within(canvasElement).findByText(/updated/);
	},
	};

notice how the story changes the value of an arg in the play function? You could write a similar story in decorators.stories.ts that adds two decorators (one conditional, one with a hook), where the conditional story switches on the arg value. Then the play function could change the arg value, causing the story to re-render itself. LMK if none of that makes sense and I can sketch out further.

a different location like adding a templates/stories dir inside of next/code/lib/preview-api?

I'm actually confused why the current preview stories live in lib/store, given that package kind of doesn't exist any more (was moved inside preview-api). @ndelangen was there a reason you didn't move the template stories too?

For now, in that lib/store directory is fine @redbugz. I'd suggest the decorators.stories.ts file seems right.

Would an e2e test go here next/code/e2e-tests/preview-web.spec.ts or somewhere else?

It would, but I am not sure an e2e test is needed, after writing my comment above I thought a bit more and I think we can get what we need with just a play function. Usually we just use e2e tests to do test the interaction between manager + preview, whereas if we just need to check behaviour when things like args change, we can get away with just an "integration" test (a template story that is tested by Chromatic + the test runner), not a full fledged playwright e2e test.

[ndelangen]

I was minimizing effort at the time @tmeasday.

Moving all that code around was complex enough as it is, and so I didn't feel the need to also change that part.

But it makes perfect sense to move those templates to the preview-api package instead, especially considering we're hoping to remove the shim packages next major release.

[tmeasday]

@redbugz any luck with this? I can take a look if you are stuck.

[tmeasday]

@redbugz I created the test here: c718894

[redbugz]

@redbugz I created the test here: c718894

Thank you. I got busy with a few things and was hoping to get back to this in a few days.

[tmeasday]

@redbugz I'm happy to merge this if you cherry pick that test across (and it passes :) )

[tmeasday]

Hmm. It looks like returning null from the decorator doesn't work in angular, lit and vue. @shilman let's figure out a pattern to get this test working.

[tmeasday]

@kasperpeulen @chakAs3 I'll note that I looked at this story in vue3 and it isn't actually possible to conditionally render there. Even with the story in this PR, the second decorator always gets rendered. It won't break this particular test, but it's not consistent with the other renderers.

We should probably write some proper stories/tests for decorator behaviour like this, and get all renderers to conform to a common contract for sanity's sake.

[chakAs3]

@kasperpeulen @chakAs3 I'll note that I looked at this story in vue3 and it isn't actually possible to conditionally render there. Even with the story in this PR, the second decorator always gets rendered. It won't break this particular test, but it's not consistent with the other renderers.

We should probably write some proper stories/tests for decorator behaviour like this, and get all renderers to conform to a common contract for sanity's sake.

Hi @tmeasday you are right I think we should skip this for vue ( renderer they don't use useEffect hook ).
for test purpose why don't we move const [counter, setCounter] = useState(0); up to have function scope, and just use setCounter, avoiding using useEffect

[chakAs3]

Hmm. It looks like returning null from the decorator doesn't work in angular, lit and vue. @shilman let's figure out a pattern to get this test working.

@tmeasday @shilman yes at least sure for vue should return modified story, but I can open a PR that handles the null/undefined value.

[chakAs3]

@shilman @tmeasday @ndelangen is it normal for this PR to have over 1500 files changed?

[tmeasday]

Hi @tmeasday you are right I think we should skip this for vue ( renderer they don't use useEffect hook ).
for test purpose why don't we move const [counter, setCounter] = useState(0); up to have function scope, and just use setCounter, avoiding using useEffect

@chakAs3 I think you misunderstand. This is not testing vue hooks, it is testing the storybook hooks which should work for every framework. So I wouldn't want to skip the test in vue.

My problem is actually that the test won't fail prior to this change in vue because it isn't possible to actually conditionally render in a decorator in the first place. This seems wrong to me but is out of scope of this PR.

yes at least sure for vue should return modified story, but I can open a PR that handles the null/undefined value.

I disagree. It should be possible to not render the story at all in a decorator. I made it work, but the interesting (broken?) part is the intervening decorator (which isn't rendered) still gets called.

is it normal for this PR to have over 1500 files changed?

Definitely not, something weird has happened when I updated the branch.

[chakAs3]

I disagree. It should be possible to not render the story at all in a decorator. I made it work, but the interesting (broken?) part is the intervening decorator (which isn't rendered) still gets called.

I agree with you, I was just telling you the current state, of course, you can choose to not render the story or render whatever you want I even demoed it to @shilman, and @kasperpeulen. but you need to return nonnull value ( ), But as you said it is something that we should revise in storybook decorator's implementation maybe in another PR.

My take is to cancel a decorator we should not have the condition inside the decorator itself because even if the decorator returns null, still we ran it first.

decorators = [ decoratorFn1() , decoratorFn2(), decoratorFn3() .....]  

my solution if it was just Vue we can run extra cycle that checks valid decorators to run the ones that don't return null
so we will have it in case the first 1 returns null

decorators = [  decoratorFn2(), decoratorFn3() ]  

But I think in React will break if you use some hooks.

NB:I dealt with these storybook hooks , they are around 9 that handle background, grid... they have useEffect hooks with caused a problem for me these decorators, i can't call them outside of the render cycle.

[tmeasday]

@chakAs3 the idea is a decorator returns what should be rendered. In react it is valid to return null as something renderable (and in other renderers too I guess), I appreciate that is not valid for the vue3 renderer, which is why I changed the story.

Alternatively, the decorator can call the passed storyFn to render the next decorator in the chain.

IMO the next decorator in the chain should not get executed if you don't call that storyFn (which is the circumstance I was trying to trigger here). Similar to the way express middleware works. That has been the behaviour of SB.

But I think in React will break if you use some hooks.

If you render the next decorator like <StoryFn/>, React will be OK with the next decorator not being called, just like if you have any old conditional component rendering. This is allowed with the rules of React hooks. In fact this is what this PR is sort of doing, treating decorator boundaries as "different components" for SB hooks.

NB:I dealt with these storybook hooks , they are around 9 that handle background, grid... they have useEffect hooks with caused a problem for me these decorators, i can't call them outside of the render cycle.

Right, and I think I called out that the decorators being called "outside rendering" was going to be an issue on that PR.

[redbugz]

Sorry, I'm back from vacation now, thanks to everyone for helping. I'm still trying to understand how the repo works, I have the latest code with the new template story. I have a few days I can help now, so willing to do whatever I can to help finish things up.
A few things I am noticing that I could use help with.

When I create a new sandbox, I am not getting the new templated story in that sandbox, even after I do all the steps in yarn task that I think I need, even Publish the packages of the monorepo to an internal npm server (publish), it's not clear to me how the sandboxes are created to know how to get the new template story to show up in a new sandbox without copy/paste.
If I copy/paste the Hooks story into the sandbox and run it in dev mode, then Interactions tab shows nothing available on the new Hooks story even though there is a play function. I noticed most of the other play functions have an expectation, so when I add any trivial expect to the play function, then the Interactions tab now show up in the story. So I think we just need to add some kind of expect at the end of the story for the play to work?

It's also not clear to me how the play function is executed by tests, or even how to run the tests locally against these templated stories, and you mentioned a chromatic test or an interaction test which I'm not familiar with and I get an error trying to run the chromatic tests locally. Do those just run during CI?

Also all of the CI checks seem to be failing on storyshots-puppeteer, not sure what I need to do to get those resolved, just continue to merge next into this branch?

[tmeasday]

Hi @redbugz you should be able to view these stories if you just run yarn start and browse to http://localhost:6006/?path=/story/lib-preview-api-decorators--hooks [1]

If I copy/paste the Hooks story into the sandbox and run it in dev mode, then Interactions tab shows nothing available on the new Hooks story even though there is a play function.

This is because only certain testing library commands are "instrumented" (so show up in the panel). Random stuff like emitting on the channel isn't. That's OK, the play function is still running.

It's also not clear to me how the play function is executed by tests, or even how to run the tests locally against these templated stories, and you mentioned a chromatic test or an interaction test which I'm not familiar with and I get an error trying to run the chromatic tests locally. Do those just run during CI?

Yes, Chromatic has run against all the sandboxes in CI.

The interaction tests is just the play function. To run it, you can just navigate to the story.

Also all of the CI checks seem to be failing on storyshots-puppeteer, not sure what I need to do to get those resolved, just continue to merge next into this branch?

Yeah the failures on this branch seem unrelated, it was working earlier, I'll dig into it.

[1] This wasn't working because it looks like maybe @ndelangen moved the stories and there was a merge conflict. I fixed that.

[tmeasday]

This story is just not rendering consistently across frameworks. @shilman I'm not sure how concerned to be. TLDR: it seems like decorators are a bit of an inconsistent mess between renderers.

[tmeasday]

@shilman OK, thanks for bearing with me. So this story "decorators: Hooks" is supposed to render component project text. It is good that it is not erroring in any framework (it would have before this PR), but it is inconsistent in Vue:

• in Vue 2 it renders blank, perhaps it isn't possible render the originalStoryFn? eg https://www.chromatic.com/test?appId=633bec4b9116cbe3b986bd53&id=64794476758af17ea832047e
• In Vue 3 it renders story component project text -- like it's rendering all decorators no matter what - e.g https://www.chromatic.com/test?appId=632066d4ff4ffcc927dd0bba&id=64794459f8b7ab17a31e605b

[shilman]

@tmeasday I spoke with @kasperpeulen about it and he's going to take a quick look at the Vue failures. My gut says we should forget about the Vue2 case and diagnose the Vue3 case / fix it if it's an easy fix.

[kasperpeulen]

@shilman @tmeasday
I tried to find a fix for Vue3, but I don't think it is easy.

I don't fully understand the decorator implementation of Vue.
I tried to recreate the decorator implementation from scratch, in a way that I understand it myself:

export function decorateStory(
  storyFn: LegacyStoryFn<VueRenderer>,
  decorators: DecoratorFunction<VueRenderer>[]
): LegacyStoryFn<VueRenderer> {
  return decorators.reduce(
    (decorated: LegacyStoryFn<VueRenderer>, decorator) => (context: StoryContext<VueRenderer>) => {
      const innerStoryFn: PartialStoryFn<VueRenderer> = (update) => {
        const sanitizedUpdate = sanitizeStoryContextUpdate(update);
        // update the args in a reactive way
        if (update) sanitizedUpdate.args = Object.assign(context.args, sanitizedUpdate.args);
        return decorated({ ...context, ...sanitizedUpdate });
      };
      const decoratedStory: VueRenderer['storyResult'] = decorator(innerStoryFn, context);
      const innerStory = () => h(innerStoryFn!);
      if (!isVNode(decoratedStory) && typeof decoratedStory === 'object') {
        decoratedStory.components = { ...decoratedStory.components, story: () => h(innerStoryFn) };
      }
      return prepare(decoratedStory, innerStory) as VueRenderer['storyResult'];
    },
    (context) => prepare(storyFn(context)) as LegacyStoryFn<VueRenderer>
  );
}

Which fixes the test, but a lot of other tests are broken.

My gut feeling is that, the inner story is basically available in 2 ways, in Vue (by calling storyFn, but also by using <story/> in your template). In this way, trying to control, the previous decorator with originalStoryFn is not going to work with Vue, as we will call storyFn anyway, to fill the value of <story>.

I can investigate this further if you think that is prio @shilman.

[shilman]

@kasperpeulen i'd say mark it with a FIXME and let's move on

[tmeasday]

I guess my final question is what is the best way to "approve" the above 5 or so vue2/3 stories that are rendering "wrong" (or at least inconsistent with the other renderers)? It feels like we should comment somewhere about it so we don't get confused later.

[shilman]

@tmeasday I have created #22945 and also commented above the story in question with a reference to the code. Merging this.

[kasperpeulen]

@shilman Probably also good to approve the chromatic snapshots with a comment to the github issue.

[shilman]

Good call @kasperpeulen . Done!

[chakAs3]

Hi. @tmeasday @shilman @kasperpeulen,
I got some time this morning to open PR , it may solve all this mystery.