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

Angular: Cannot render same story more than once in docs #15170

Closed
dexster opened this issue Jun 8, 2021 · 11 comments · Fixed by #15501
Closed

Angular: Cannot render same story more than once in docs #15170

dexster opened this issue Jun 8, 2021 · 11 comments · Fixed by #15501

Comments

@dexster
Copy link
Contributor

dexster commented Jun 8, 2021

Describe the bug
When we need to show a story more than once in the mdx docs then only the first one shows

To Reproduce

ng g new sb-ivy
cd sb-ivy
npx sb@next init 

Remove the default export from Button.stories.ts

Add an mdx page to the button story

<Meta title="Example/Button"
  component={ButtonComponent}
/>

<Canvas>
  <Story story={stories.Primary}/>
</Canvas>

<Canvas>
  <Story id="example-button--primary" />
</Canvas>

npm run storybook

Click the Docs tab and notice the second Story is not rendered.

Screenshot 2021-06-08 at 09 27 17

System
Angular 12.0.2

System:
OS: macOS 11.4
CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
Binaries:
Node: 14.15.5 - ~/.nvm/versions/node/v14.15.5/bin/node
Yarn: 1.22.10 - ~/.nvm/versions/node/v14.15.5/bin/yarn
npm: 6.14.11 - ~/.nvm/versions/node/v14.15.5/bin/npm
Browsers:
Chrome: 91.0.4472.77
Edge: 90.0.818.66
Firefox: 88.0.1
Safari: 14.1.1
npmPackages:
@storybook/addon-actions: 6.3.0-beta.17 => 6.3.0-beta.17
@storybook/addon-essentials: 6.3.0-beta.17 => 6.3.0-beta.17
@storybook/addon-links: 6.3.0-beta.17 => 6.3.0-beta.17
@storybook/angular: 6.3.0-beta.17 => 6.3.0-beta.17
@storybook/builder-webpack5: 6.3.0-beta.17 => 6.3.0-beta.17
@storybook/manager-webpack5: 6.3.0-beta.17 => 6.3.0-beta.17

Additional context
Add any other context about the problem here.

@ThibaudAV
Copy link
Contributor

The problem is that the 2 stories have the same id.
And the generation of the angular doc uses it as a unique id.
The parent div added by the docs also uses the id as an html attribute.

image

So I don't know if it's a bug of the docs addon to fix or if it is necessary to add a random id only for angular 🤷‍♂️

@dexster
Copy link
Contributor Author

dexster commented Jun 8, 2021

I will look more into this. It does work in my prod app where I show the same stories multiple times in docs

@ThibaudAV
Copy link
Contributor

For info : I did reproduce the bug with the storybook example project. And it doesn't surprise me at all that it worked before and not anymore.

With the addition of a random id for angular it can be fixed quickly. but i don't know if it's the right thing to do 🤔

@shilman shilman added the angular label Jun 8, 2021
@dexster
Copy link
Contributor Author

dexster commented Jun 8, 2021

The id is currently there for beta.6 (seen below) and previous so the removal of the id is a breaking change for my scenario. Would it be difficult to get this id back? I think it's the right thing to do 😄

example-button--primary_0
example-button--primary_1

<div id="story--example-button--primary">
  <example-button--primary_0 ng-version="12.0.3">
    <storybook-button _nghost-quv-c45="" ng-reflect-primary="true" ng-reflect-size="medium"> 
       ...
    </storybook-button>
  </example-button--primary_0>
</div>

<div id="story--example-button--primary">
  <example-button--primary_1 ng-version="12.0.3">
    <storybook-button _nghost-quv-c45="" ng-reflect-primary="true" ng-reflect-size="medium"> 
       ...
    </storybook-button>
  </example-button--primary_1>
</div>

@dexster
Copy link
Contributor Author

dexster commented Jun 8, 2021

@ThibaudAV Appending an id to the targetSelector in the AbstractRenderer class does fix the issue. Could I submit a PR or is there something you want to look at first?

@ThibaudAV
Copy link
Contributor

ThibaudAV commented Jun 8, 2021

arf no. this is not a good solution :/ . Otherwise the optimization to not render the whole angular app when only args change doesn't work anymore (optimization only for the canvas)

But something like this :
here

export const prepareForInline = (storyFn: StoryFn<IStory>, { id, parameters }: StoryContext) => {
  const randomId = Math.random().toString(36).substring(7); // <------
  return React.createElement('div', {
    ref: async (node?: HTMLDivElement): Promise<void> => {
      if (!node) {
        return null;
      }

      return limit(async () => {
        const renderer = await rendererFactory.getRendererInstance(`${id}-${randomId}`, node); // <------ 
        await renderer.render({
          forced: false,
          parameters,
          storyFnAngular: storyFn(),
          targetDOMNode: node,
        });
      });
    },
  });
};

should be enough

but I think that for the future it would be better if the doc addon is in charge of giving a real unique id. even for other frameworks I think it would be better 🤷‍♂️

@shilman
Copy link
Member

shilman commented Jun 8, 2021

cc @IgorSzyporyn ☝️ another unique ID use case. @ThibaudAV we were discussing https://www.npmjs.com/package/nanoid

@dexster
Copy link
Contributor Author

dexster commented Jun 28, 2021

I haven't made any updates to the angular code. Will this be done in the doc addon or should i try again just to fix Angular ?

I updated to 6.3 final and my docs look very empty now because of this 😞

@IgorSzyporyn
Copy link
Contributor

cc @IgorSzyporyn ☝️ another unique ID use case. @ThibaudAV we were discussing https://www.npmjs.com/package/nanoid

I like nanoid, 108 bytes (minified and gzipped), has no dependencies and is a lot faster than UUID (If you are to believe the propaganda)

@IgorSzyporyn
Copy link
Contributor

cc @IgorSzyporyn ☝️ another unique ID use case. @ThibaudAV we were discussing https://www.npmjs.com/package/nanoid

#15415

@shilman
Copy link
Member

shilman commented Jul 9, 2021

Yowza!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.4.0-alpha.13 containing PR #15501 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants