storybookjs / storybook

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

[Bug]: Angular provider recreation differs between docs & story view #29061

Open twittwer opened 3 weeks ago

twittwer commented 3 weeks ago

Describe the bug

When defining Angular providers in the moduleMetadata of the render function, the recreation of them differs between the docs and the story page. In the docs page, they get recreated on every change of the Controls, but this doesn't happen on the story page.

We need this recreation to mock values of services or e.g. NgRx Store based on Storybook Controls.

Reproduction link

https://stackblitz.com/edit/github-yzh9ef?file=src%2Fstories%2Fbutton.stories.ts

Reproduction steps

  1. Open the Storybook from the Reproduction.
  2. Go to docs page of the Button component.
  3. Change input & service value via controls.
    -> Both values are updated in the story view.
  4. Go to the single story of the Button component.
  5. Change input & service value via controls.
    -> Only input value changes are represented in the story view and service value changes doesn't have any effect.

System

System:
    OS: Windows 10 10.0.19045
    CPU: (12) x64 12th Gen Intel(R) Core(TM) i7-1265U
  Binaries:
    Node: 20.17.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.8.2 - C:\Program Files\nodejs\npm.CMD <----- active
  Browsers:
    Edge: Chromium (127.0.2651.74)
  npmPackages:
    @storybook/addon-designs: 8.0.3 => 8.0.3 
    @storybook/addon-essentials: 8.2.9 => 8.2.9 
    @storybook/addon-interactions: 8.2.9 => 8.2.9 
    @storybook/addon-mdx-gfm: 8.2.9 => 8.2.9 
    @storybook/addon-themes: 8.2.9 => 8.2.9 
    @storybook/angular: 8.2.9 => 8.2.9 
    @storybook/core-server: 8.2.9 => 8.2.9 
    @storybook/manager-api: 8.2.9 => 8.2.9 
    @storybook/test-runner: 0.19.1 => 0.19.1 
    @storybook/theming: 8.2.9 => 8.2.9 
    eslint-plugin-storybook: 0.8.0 => 0.8.0 
    storybook: 8.2.9 => 8.2.9

Additional context

No response

Marklb commented 2 weeks ago

This is working as expected, based on Storybook's implementation, but there could possibly be an improvement to make Storybook's implementation more smart.

I don't remember why the docs fully rerender the story on each Control change, but typically Controls are assumed to be an input, so Storybook acts like only the input changed. There is a simple check to try and determine if the story should fully rerender do to various changes that can't update dynamically. That logic was implemented before the existence of signals and your scenario is one that I hadn't considered, yet. If you were using the value, instead of wrapping it in a signal then it should have worked as you expected, but you of course don't want to change your component, just for Storybook. So, I will provide a workaround, until a better solution is decided.

One condition Storybook uses to determine a full rerender is if anything in the moduleMetadata changed. That check isn't a thorough deep compare of each individual property. It uses telejson to stringify the object and compare it to the previous as a simple fast solution. telejson is more thorough than JSON.stringify, but it still doesn't handle stringifying a signal to anything distinguishable. So, if you stringify { value: signal(4) } then you get "{}", since a signal stringifies to undefined.

Workaround 1:

This is not ideal, since I am having to modify the specific provider, but the concept could possibly be implemented as a generic wrapper for your providers. I just haven't had a chance to look into a good way to check if an object is a signal, yet. One side-effect is this could break your story if you are stringifying the provider in your app and rely on the existing way it stringifies.

moduleMetadata: {
  providers: [
    {
      provide: Service,
      useValue: {
        value: signal(args.serviceValue),
        // @ts-ignore
        toJSON: function() {
          return {
            ...this,
            value: this.value.toString(),
          }
        },
      } as const satisfies Service,
    },
  ],
},

Workaround 2:

You can create a decorator that forces a full rerender on any arg change, by adding a unique provider each time the decorator is called. This makes it more naive and causes it to not care if anything actually changed, but it is generic and can be dropped in per story or globally.

let _tmp = 0
const rerenderAlways = (storyFn: any) => {
  const story = storyFn()
  if (_tmp >= Number.MAX_VALUE - 1) {
    _tmp = 0
  }
  return {
    ...story,
    moduleMetadata: {
      ...(story?.moduleMetadata || {}),
      providers: [
        ...(story?.moduleMetadata?.providers || []),
        { provide: '__STORY_WORKAROUND__', useValue: _tmp++ },
      ],
    },
  }
}

const meta: Meta<ButtonComponent & { serviceValue: number }> = {
  ...,
  decorators: [
    rerenderAlways,
  ],
};
export default meta;
twittwer commented 2 weeks ago

Thank you @Marklb for this detailed explanation 🙏

It is good to know, how the rerender is triggered and I like your workaround with the decorator, because I was already looking for a forceRerender flag 🙂

The first option looks like a more explicit approach to the problem and with the mentioned wrapper it looks clean as well.

The correct serialization of signals seems to be the best approach for a general feature, to resolve this use case.

Marklb commented 2 weeks ago

@twittwer I will think about it, but I don't think there is a correct serialization of a signal. The more I have thought about it, I think it makes sense that Angular doesn't provide toJSON, like the one in my first workaround.

In your case, you pass a new signal instance each time, so the current value is a valid serialization. That isn't always the case, though. You could pass the same signal instance multiple times and call set() or update() in the story, which would change the result of toString(), but that doesn't mean it isn't the same signal.

Ideally, we should be able to compare the object instances, but I don't know if there is a good way to do that and not accidentally cause a memory leak by retaining references. Maybe WeakMap could avoid the memory leak, but I don't know if the performance would take a hit from adding that additional iteration over objects. I am busy with other stuff, but I will give it a try if I have time, unless someone else wants to do it.