MDX Angular stories get re-instantiated within the same docs tab by navigating back and forth

[kroeder]

Describe the bug

If you have a structure like this

Button MDX Story
- Story A
- Story B

I would expect that after clicking Story A or B the docs tab gets rendered only once.

The actual behaviour is that if you enter Story A and then Story B all of the Angular instances get destroyed and re-bootstrapped which is fine for smaller components but for complex / data-intensive stories, this is a major performance impact.

I don't know how other framework implementations handle this but I would expect navigating inside the same rendered content should do nothing.

To Reproduce

• Start example/angular-cli in the monorepo
• Add a breakpoint / console.log right before await getPlatform().bootstrapModule in app/angular/src/client/preview/angular-beta/AbstractRenderer.ts

System

Environment Info:

  System:
    OS: macOS 12.3.1
    CPU: (10) arm64 Apple M1 Pro
  Binaries:
    Node: 16.15.0 - ~/.nvm/versions/node/v16.15.0/bin/node
    Yarn: 3.1.1 - ~/.nvm/versions/node/v16.15.0/bin/yarn
    npm: 8.5.5 - ~/.nvm/versions/node/v16.15.0/bin/npm
  Browsers:
    Chrome: 101.0.4951.54
    Safari: 15.4
  npmPackages:
    @storybook/addon-storyshots: 6.5.0-beta.6 => 6.5.0-beta.6 
    @storybook/angular: 6.5.0-beta.6 => 6.5.0-beta.6 

Additional context

I believe the described behaviour above is the correct behaviour for the Canvas-Tab.
The render function needs to know if the navigation was triggered while being in the docs tab and then do nothing instead of destorying / bootstrapping everything

[Yogu]

We found another problem with angular stories in docs: They seem to be instantiated multiple times for one docs load (seven times in our case). This makes the initial load very slow, in addition to the re-render on navigation clicks.

To me it seems like the code in AbstractRenderer.render that checks whether a full re-render is required is broken and always returns true

• Some time ago, a random identifier has been added to angular storyIds (9db2e2b). This means that the AbstractRenderer is re-instantiated for every docs render, so it can't cache the angular module
• the code in fullRenderRequired seems to interpret the forced argument inverse (it calls for a re-render if forced is false)
• but when I "fix" these two things, stories are only rendered once, the partial re-render fails. It seems like it needs to attach an element somewhere but it only updates the props.

[Yogu]

this code here also looks like it could cause problems

6ae4ef0

storybook/app/angular/src/client/preview/angular-beta/DocsRenderer.ts

Lines 16 to 37 in 8da2f7f

	/**
	* Destroy and recreate the PlatformBrowserDynamic of angular
	* For several stories to be rendered in the same docs we should
	* not destroy angular between each rendering but do it when the
	* rendered stories are not needed anymore.
	*
	* Note for improvement: currently there is one event per story
	* rendered in the doc. But one event could be enough for the whole docs
	*
	*/
	channel.once(Events.STORY_CHANGED, async () => {
	await DocsRenderer.resetPlatformBrowserDynamic();
	});
	/**
	* Destroy and recreate the PlatformBrowserDynamic of angular
	* when doc re render. Allows to call ngOnDestroy of angular
	* for previous component
	*/
	channel.once(Events.DOCS_RENDERED, async () => {
	await DocsRenderer.resetPlatformBrowserDynamic();
	});

but even If I remove that, the story contents are still constantly being cleared, even within this await here

storybook/app/angular/src/client/preview/angular-beta/AbstractRenderer.ts

Line 140 in 8da2f7f

await getPlatform().bootstrapModule(

That's also what is causing these errors for us

ERROR Error: The selector "components-data-table--column-widths" did not match any elements
    at DefaultDomRenderer2.selectRootElement (platform-browser.mjs:574:19)
    at BaseAnimationRenderer.selectRootElement (animations.mjs:261:30)
    at locateHostElement (core.mjs:9831:25)
    at ComponentFactory.create (core.mjs:21561:13)
    at ApplicationRef.bootstrap (core.mjs:26469:42)
    at core.mjs:26154:64
    at Array.forEach (<anonymous>)
    at PlatformRef._moduleDoBootstrap (core.mjs:26154:44)
    at core.mjs:26124:26
    at _ZoneDelegate.push../node_modules/zone.js/dist/zone.js._ZoneDelegate.invoke (zone.js:409:1)

I think I'll stop here :D but maybe these findings help

[MichaelArestad]

Thank you for such a stellar bug report @kroeder and @Yogu! @storybookjs/angular can you take a look?

[kroeder]

I found out that this seems to be fixed using

  // main.js
  features: {
    modernInlineRender: true
  },

However, since this is not stable yet, other bugs are popping up. As soon as you switch between 2 docs stories, all Angular components just break and Storybook renders nothing

Probably a good idea to fix the new InlineRenderer instead of optimizing the old one? 🤔

[tmeasday]

@kroeder I definitely think we should focus on the new inline render, we aren't sure if we will even keep the old one in 7.0.

[sir-captainmorgan21]

@tmeasday @kroeder is there any update on this? Not being able to use the inline renderer reliably is kind of a bummer. Thanks for looking into this btw!

[tmeasday]

Hey @sir-captainmorgan21 in SB7 we are only using the new inline renderer, and there have been many improvements. I'd suggest trying that out when you can!

[Marklb]

I haven't kept up with the SB7 docs changes, but will this issue's scenario still be something that can happen in SB7? Unless it can still be configured to have a Canvas and Docs tab, clicking a story will always go to that story's Canvas page now, right?

[tmeasday]

I guess not @Marklb. Certainly not as easily. You could create two different docs pages that contain the same set of stories and click between them I suppose.

[sir-captainmorgan21]

@tmeasday ok awesome. Will try that out. With SB7 Im assuming you no longer have to configure inlineStories: true in the preview.js anymore? Seems so since the inline renderer is the only one being used now.

Also, I think this may be related to and resolve this issue: #14026

Do you mind confirming? I know it would help a whole lot of folks who are working around it by just turning off inlineStories

[tmeasday]

Yes, I think so based on this example from one of our sandboxes (no special parameters/settings): https://6377871ac35b4b579c6b8bfc-fbuoibhfqz.chromatic.com/?path=/docs/addons-docs-docs2-button--docs

[Marklb]

I guess not @Marklb. Certainly not as easily. You could create two different docs pages that contain the same set of stories and click between them I suppose.

If I understand what you are saying, then that would be a different scenario that I would think should rerender.

Depending on the extent of this issue, maybe it would still apply. I always thought of clicking a Story from the sidenav or the links addon as opening a new docs page that happens to be the same content as the other stories in the file. If that is what is actually happening then I figured this issue would change it to be able to recognize the same docs page is being navigated to and scroll to the story instead.

I forgot stories could be defined in an mdx file, because I always define them in a ts file even if I decide to render them in a mdx file later. This makes a lot more sense if thinking about it with stories defined in an mdx file.

@tmeasday When you say "in SB7 we are only using the new inline renderer", do you mean your team is only using it or it is the only one that will be usable? I assume you mean your team is only using it, but the option of using an iframe will still be available. The inline option is preferred for most of my library components, but in an application that has more larger "page-like" components I prefer the isolation of an iframe. I also tried some that use CSS media queries and they did not look right when resized into a container element.

[tmeasday]

If I understand what you are saying, then that would be a different scenario that I would think should rerender.

@Marklb I think you probably have that right I'm not 100% clear on the circumstances that led to the original error here.

@tmeasday When you say "in SB7 we are only using the new inline renderer", do you mean your team is only using it or it is the only one that will be usable?

I think there's some ambiguity here, there are two things that are involved:

1. The modernInlineRender flag which changed the way inline rendering works. That was an opt-in 6.x but is the only option in 7.x.

2. Rendering stories inline vs in iframes. I'm not sure what the default was in 6.x (for Angular) but from what I can tell, in 7.x the default is to render inline. However you can still use the docs.inlineStories parameter to change this.

[shilman]

Ta-da!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.14 containing PR #20118 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.