storybookjs / storybook

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

[Bug]: Converting circular structure to JSON #25275

Closed timtos closed 3 weeks ago

timtos commented 8 months ago

Describe the bug

When working with data structures with parent-child-relationships, one gets a "Converting circular structure to JSON" error:

Converting circular structure to JSON
    --> starting at object with constructor 'ButtonComponent'
    |     property 'children' -> object with constructor 'Array'
    |     index 0 -> object with constructor 'ButtonComponent'
    --- property 'parent' closes the circle

This was not happening in Storybook 6. To disable some configuration via argTypes didn't help:

 argTypes: {
    children: {
      table: false,
      control: false,
      //   mapping: {
      //     parent: undefined,
      //     children: []
      // },
    },
  },

To Reproduce

Open the following link, you should directly see the error message after you visited the first button story:

image

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

System

Using the reproduction link:
Storybook Environment Info:

  System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.4.2 - /usr/local/bin/npm <----- active
    pnpm: 8.10.5 - /usr/local/bin/pnpm
  npmPackages:
    @storybook/addon-docs: ^8.0.0-alpha.3 => 8.0.0-alpha.3 
    @storybook/addon-essentials: ^8.0.0-alpha.3 => 8.0.0-alpha.3 
    @storybook/addon-interactions: ^8.0.0-alpha.3 => 8.0.0-alpha.3 
    @storybook/addon-links: ^8.0.0-alpha.3 => 8.0.0-alpha.3 
    @storybook/angular: ^8.0.0-alpha.3 => 8.0.0-alpha.3 
    @storybook/blocks: ^8.0.0-alpha.3 => 8.0.0-alpha.3 
    @storybook/test: ^8.0.0-alpha.3 => 8.0.0-alpha.3 
    storybook: ^8.0.0-alpha.3 => 8.0.0-alpha.3 

On my machine:
Storybook Environment Info:

  System:
    OS: Windows 10 10.0.19045
    CPU: (16) x64 Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz       
  Binaries:
    Node: 20.10.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - C:\Program Files\nodejs\yarn.CMD <----- active
    npm: 10.2.3 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Chromium (120.0.2210.77)
  npmPackages:
    @storybook/addon-a11y: ~7.5.3 => 7.5.3 
    @storybook/addon-actions: ~7.5.3 => 7.5.3
    @storybook/addon-controls: ~7.5.3 => 7.5.3
    @storybook/addon-designs: ~7.0.5 => 7.0.5
    @storybook/addon-docs: ~7.5.3 => 7.5.3
    @storybook/addon-essentials: ~7.5.3 => 7.5.3
    @storybook/addon-storysource: ~7.5.3 => 7.5.3
    @storybook/addons: ~7.5.3 => 7.5.3
    @storybook/angular: ~7.5.3 => 7.5.3
    @storybook/api: ~7.5.3 => 7.5.3
    @storybook/blocks: ~7.5.3 => 7.5.3
    @storybook/components: ~7.5.3 => 7.5.3
    @storybook/core-events: ~7.5.3 => 7.5.3
    @storybook/core-server: ~7.5.3 => 7.5.3
    @storybook/theming: ~7.5.3 => 7.5.3
    eslint-plugin-storybook: ~0.6.15 => 0.6.15
    storybook-multilevel-sort: ~2.0.0 => 2.0.0

Additional context

I understand, that the problem is with the JSON serializer as it cannot handle circular structures, but Storybook 6 could somehow handle this situation so it would be great, if storybook could be more robust again in these kind of scenarios.

Unfortunately, I didn't understand how to save the day by using the mapping property.

Side note:

With

docs: {
   source: {
      type: 'code',
   },
},

I don't get an error, but then no updates and no output is working anymore, so I skipped this again quickly as this is even worse. Any ideas?

abhijitro commented 5 months ago

can i work on this?

timtos commented 5 months ago

Oh, that would be so awesome. Most of my stories have this problem and it is so annoying to always have to reload the page to see the actual story. If I can assist with any information, tell me. And thank you.

abhijitro commented 5 months ago

I guess i am not getting this bug in my local, can you elaborate on where exactly in your project ,you are getting this bug?

timtos commented 5 months ago

Hey Abhijit, are you getting the bug here?

Marklb commented 5 months ago

I don't think this issue is a bug, necessarily. It is expected.

There is a bug with how Storybook is mapping the inputs, though. (At least, I think I am correct about this.) When it is mapping the inputs to a readable value that is shown in the "Show Code" snippet, objects get stringified and JSON.stringify doesn't handle circular references. I thought object handling had been changed for that, but I will need to find the issue that I pointed this out in and see if it is still open.

You mentioned trying to use the mapping, so I think you had the correct idea and got thrown off by the issue I mentioned above.

Just to point out what is wrong in the Stackblitz:

// button.stories.ts
...
const btn0 = new ButtonComponent();
const btn1 = new ButtonComponent();

btn0.children = [btn1];
btn1.parent = btn0;

const children = [btn0, btn1];

// More on writing stories with args: https://storybook.js.org/docs/writing-stories/args
export const Primary: Story = {
  args: {
    primary: true,
    label: 'Button',
    children,
  },
};

That snippet will not work, without some sort of mapping, because you can't serialize an instance to a string and deserialize it back to the instance. I don't remember how Storybook v6 worked, but I don't think this worked in it either, but it may not have thrown an error and the object you used may have deserialized enough of the object that you didn't notice that it wasn't the original instance.

Workaround to use the mapping

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

This will prevent the inputs from being mapped to a readable value in the docs "Show Code" snippet, but the story should be generated very similar to what Storybook would have auto generated for you:

// button.stories.ts
...
const btn0 = new ButtonComponent();
const btn1 = new ButtonComponent();

btn0.children = [btn1];
btn1.parent = btn0;

const children = [btn0, btn1];

// More on how to set up stories at: https://storybook.js.org/docs/writing-stories
const meta: Meta<ButtonComponent> = {
  title: 'Example/Button',
  component: ButtonComponent,
  argTypes: {
    children: {
      table: false,
      control: false,
      options: ['Children 1'],
      mapping: {
        'Children 1': children,
      },
    },
    parent: {
      table: false,
      control: false,
      options: ['Parent 1'],
      mapping: {
        'Parent 1': parent,
      },
    },
  },
  render: (args) => ({
    props: args,
    // template: `<storybook-button [primary]="primary" [label]="label" [children]="children" [parent]="parent"></storybook-button>`,
    template: `<storybook-button ${argsToTemplate(args)}></storybook-button>`,
  }),
};

export default meta;
type Story = StoryObj<ButtonComponent>;

// More on writing stories with args: https://storybook.js.org/docs/writing-stories/args
export const Primary: Story = {
  args: {
    primary: true,
    label: 'Button',
    children: 'Children 1' as any, // I used any, because of how the mapping works. There is probably a snippet showing how to adjust the types better, but I would need to find one.
    parent: 'Parent 1' as any,
  },
};

In this solution, I am bypassing Storybook's auto template generation, by providing my own. Storybook provides a method argsToTemplate as a helper to generate a string for the inputs/outputs. It has some limitations that could be better, but should work fine in most scenarios. The line that I commented out is also an option, if you need to manually build the template, but it is more boilerplate to maintain.

timtos commented 5 months ago

Hey Mark, thank you for your comment. I'll try your workaround! The whole thing was working in the old version 6 of storybook and it is annoying, that it is now not working anymore. So even if it is not a "real" bug, perhaps we can make storybook a little bit more forgiving. :) But first, I'll try your workaround. Thanks again.

timtos commented 5 months ago

Mmh, unfortunately, I don't get it running ... The example is a little bit too simple; I have an object "config" and somewhere within this object, there could be "children" or "fields" or "items" that are referencing other "children" or "fields" or "items". And actually, I totally don't care about these in my story. I don't want to manipulate them, I don't want to show the "config". The best would be, if storybook would just ignore the input property when I set control to false. Or do what the v6 was doing (or at least it seems so) - just ignore any json errors. Today, v8 was released so let's see if this brings a cure.

If anybody has some more ideas or thoughts, I would be very grateful. Thank you.

joshuanathanson commented 3 months ago

I'm in the same boat as @timtos - and I'm on version 8. Interestingly, the crash only happens when I move my mouse over the row that has the control with the circular json. I guess hovering would cause something in Storybook to call JSON.stringify().

I'd also like it if you could disable or hide the control for a property by setting control: false in argTypes. Ideally this wouldn't ever be required, but at least it could be a workaround until the actual issue is addressed.

timtos commented 1 month ago

The error seems to be in the createAngularInputProperty. There a JSON.stringify is called if the arg is an object. And as it should be allowed to have an object with parent/child relationship with a child point back to its parent, I think this is a bug or should at least be handled differently. And as it was working before, it would be great if this would work again.

It seems that the stringify is done always, even if I exclude the argument and set it to control: false. Any updates on this? Somebody found a workaround?

Marklb commented 1 month ago

@timtos I finally got around to updating my environment to try and knock out some of these issues that I described the problem, without getting around to fixing it. #28498 should fix the error, but I am open to suggestions on improvements to the solution I went with.

As for a workaround, until that PR is ready to merge, I don't know of a good workaround, but https://github.com/storybookjs/storybook/issues/16855#issuecomment-1900540330 seems to provide a possible workaround. I don't like that it modifies the input object, which could affect the story, but if you don't mind overwritting toJSON then it is a small workaround.

timtos commented 1 month ago

@Marklb I am facing some issues with the workaround but perhaps it is now even best to wait for your greatly appreciated work.