Addon-docs: Error handling for invalid Story id

[atanasster]

Issue: #7964

What I did

added a check for no children to Story.tsx
added an empty story example

How to test

<Preview>
  <Story id="empty-story" />
</Preview>

[vercel]

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-atanasster-mdx-empty-story.storybook.now.sh

[shilman]

@atanasster Thanks for this PR! It's the right idea, but I think the implementation needs work. Comments below.

[atanasster]

@shilman added a renderStoryFn to take care both of invalid storyFn and also catch exception from using react hooks.
Tried any other recommended method (https://overreacted.io/how-does-react-tell-a-class-from-a-function/) to determine if a function is actually a react component but they didn't seem to work with the empty/invalid story.
If you have other ideas to try let me know, just this was the best I could come up at this moment.

[shilman]

Thanks @atanasster! So would you consider this a bugfix for the hooks support PR? If so, I'll try to get it merged tomorrow my time.

[atanasster]

@shilman yes it can be considered a fix for the hooks support and I think it should be merged.
From what I saw, there is a default storyFn that is not a react function/component that was used with the invalid story.
So this is both a safer way to enable react hooks (now using react magic only as a fallback), but also allow for storyFn functions that are not really react, so it should probably be better for non-react frameworks used to write inline stories.

[shilman]

@atanasster I tried the latest version on this PR with a clean bootstrap and am seeing the following issues on the Docs tab in official-storybook:

1. Addons|Docs/mdx: The nonexistent story does not cause the page to crash. However no error is shown either.
2. Docs|Story: The docs page crashes. Since this component is special, I'm not sure if this matters.
3. Others|Hooks: The docs page crashes. I think this should work.

The error in all crashes is: Invalid hook call. Hooks can only be called inside of the body of a function component.

[atanasster]

@shilman ouch, very sorry about that. It seems the exception handler might be too specific for your config and you might have a different exception name if (e.name === 'Invariant Violations') in Story.tsx ? Can you try to just comment out the check for the e.name?

[atanasster]

Sorry - a misspelling crept in - 'Invariant Violation' was spelled 'Invariant Violations'. Maybe I pressed ctrl+s to save the file or something, because it was working for me before.
Pushed the fixed spelling now, sorry again.

[shilman]

@atanasster no worries. Fixing the typo fixe the hooks issues on my machine. Thanks! However, i'm still seeing the following on Addons|Docs/mdx:

Does it work on your machine?

[atanasster]

@shilman glad to hear it fixes the issue.

As for the empty message, no it doesn't show here, I didn't re-add it (I just added a fix for the exception if storyFn was null) - since the empty messages were in the other file (Story.tsx I think?)instead of the InlineStory?

[atanasster]

Pushed again, can you check the wording of the message

Removed the unused children /weird thing to say in anything but react programming :)/

Shouldn't the linter catch those btw?

[shilman]

@atanasster This is working well for me now, thanks for your patience!

I worry a little bit about the hardcoded hooks error--I wonder why can't we just call <StoryFn /> (equivalent to React.createElement?) every time.

[shilman]

Also, why do we need the try/catch at all? Isn't the non-existent story handled by the check at the beginning of the function?

[atanasster]

@shilman
The try catch is currently for react hooks, to fallback on react magic
I also don't like the hardcoded error handling, but the situation made me worried about always using react rendering function and potentially breaking stories that used to work under non-react environments. Tried to find any other way to determine if its actually a react component to be rendered or not, but only the try/catch seemed to always work.
Your call on this one?

[shilman]

What happens if we just always run <StoryFn />? Does it break other frameworks? It didn't seem to when you introduced it in your other PR.

[atanasster]

• For just <StoryFn /> - I can't yet run and test the other frameworks, so I just chickened out :) Also, it was only needed for React hooks, storyFn() works fine for class components and it works fine with storybooks hooks (the hooks example in the official storybook example, where I added a react hooks example that will have problems).

[shilman]

@atanasster Why not just make this React.ElementType? It will clean up the typing below too.

[shilman]

And you can do everything inline too

[atanasster]

thanks for the feedback - simplified the code now

[shilman]

Awesome!