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

[Bug]: Decorators causes source snippets in docs to show <No Display Name /> #21649

Closed
JReinhold opened this issue Mar 17, 2023 · 7 comments
Closed

Comments

@JReinhold
Copy link
Contributor

Describe the bug

If you have a decorator, the source snippet in docs mode will not show the story's source, but instead the decorator with a <No Display Name /> child.

This bug was likely introduced by #21182, as applying

features: {
  legacyDecoratorFileOrder: true,
}

makes it work again.

To Reproduce

No response

System

No response

Additional context

https://chromaticqa.slack.com/archives/C03BNCJ8C7K/p1679047287634789

v7.0.0-rc.3 - Vite

image (5)

v7.0.0-beta.54 - Vite

image (6)

v6.5.9 - CRA

https://5f84c5baa35bdd0022a7684f-mvszeeebuw.chromatic.com/?path=/docs/components-restaurantcard--default
image (4)

@JReinhold
Copy link
Contributor Author

We agreed to default excludeDecorators to true in all renderers (or more likely in the core implementation?), but that wouldn't actually fix this issue, so we'll keep this open, but most likely not Required for GA.

tmeasday added a commit that referenced this issue Mar 22, 2023
Due to reordering in #21182 the source decorator comes *before* any user decorator in `preview.js`. This means that we commonly trigger the bug in #21649.

Really it makes more sense to default this to true and people can opt out in cases where they want to show the decorator.
@tmeasday
Copy link
Member

tmeasday commented Mar 22, 2023

Here's the PR to do that: #21722

Do we have any idea where this actual issue stems from? It's not correct to say

This bug was likely introduced by #21182

as I mentioned on #21722, that change actually fixed a bug that you couldn't previously see decorators you'd added in preview.js, such as the <Provider> from Mealdrop above. If you have exlcudeDecorators = false, you should see the decorators!

The real bug here is why you can't see any deeper inside the provider decorator, and it says <No Display Name />. Do we have any idea where that comes from?

In any case, I agree this should be dropped from Required from GA now.

@tmeasday
Copy link
Member

Do we have any idea where that comes from?

Ok, I think the reason is that the decorator in question does something like:

(Story) => <div><Story /></div>

I guess React sees the <Story /> and can't figure out a display name, for whatever reason. But the deeper issue here is that I suppose our JSX decorator doesn't go any further into the element tree than one level. That's fundamentally incompatible with showing decorators isn't it?

If you change the decorator to

(Story) => <div>{Story()}</div>

image

It works, but that's not really an idiomatic React decorator.

@shilman shilman moved this from Required for GA to Nice to have in Core Team Projects Mar 22, 2023
@shilman
Copy link
Member

shilman commented Mar 24, 2023

@tmeasday I'm not sure how react-element-to-jsx-string works under the hood & how easy it would be to add recursion support, but it's not a library option so it would be a (possibly small) project to get it working

@tmeasday
Copy link
Member

tmeasday commented Mar 25, 2023

OK. I wonder if we should consider dropping the option entirely for React then, given the value of "showing decorators and not the story itself" seems pretty low.

I guess some decorators might use the Story() syntax rather than <Story/> so there could be cases where it is useful 🤷

@tmeasday
Copy link
Member

tmeasday commented May 3, 2023

I think realistically we are not going to update the library to look "inside" React elements, so I would close this as WONTFIX, unless there is a simper fix we can do to change <No Display Name /> to <Storybook Decorator /> or something. Thoughts @shilman @JReinhold ?

@vanessayuenn
Copy link
Contributor

Closing this as per ☝🏼 , but please let me know if this should be reopened!

@vanessayuenn vanessayuenn closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Core Team Projects May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants