Merge pull request #24982 from Marklb/marklb/fix-multi-ng-app-rendering

Angular: Remove cached NgModules and introduce a global queue during bootstrapping

code/frameworks/angular/src/client/angular-beta/AbstractRenderer.ts

@@ -7,8 +7,8 @@ import { stringify } from 'telejson';
 import { ICollection, StoryFnAngularReturnType } from '../types';
 import { getApplication } from './StorybookModule';
 import { storyPropsProvider } from './StorybookProvider';
-import { componentNgModules } from './StorybookWrapperComponent';
 import { PropertyExtractor } from './utils/PropertyExtractor';
+import { queueBootstrapping } from './utils/BootstrapQueue';
 
 type StoryRenderInfo = {
   storyFnAngular: StoryFnAngularReturnType;
@@ -30,35 +30,13 @@ export abstract class AbstractRenderer {
    * Wait and destroy the platform
    */
   public static resetApplications(domNode?: HTMLElement) {
-    componentNgModules.clear();
     applicationRefs.forEach((appRef, appDOMNode) => {
       if (!appRef.destroyed && (!domNode || appDOMNode === domNode)) {
         appRef.destroy();
       }
     });
   }
 
-  /**
-   * Reset compiled components because we often want to compile the same component with
-   * more than one NgModule.
-   */
-  protected static resetCompiledComponents = async () => {
-    try {
-      // Clear global Angular component cache in order to be able to re-render the same component across multiple stories
-      //
-      // References:
-      // https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L50
-      // https://github.com/angular/angular/blob/2ebe2bcb2fe19bf672316b05f15241fd7fd40803/packages/core/src/render3/jit/module.ts#L377-L384
-      const { ɵresetCompiledComponents } = await import('@angular/core');
-      ɵresetCompiledComponents();
-    } catch (e) {
-      /**
-       * noop catch
-       * This means angular removed or modified ɵresetCompiledComponents
-       */
-    }
-  };
-
   protected previousStoryRenderInfo = new Map<HTMLElement, StoryRenderInfo>();
 
   // Observable to change the properties dynamically without reloading angular module&component
@@ -77,8 +55,6 @@ export abstract class AbstractRenderer {
 
   protected abstract beforeFullRender(domNode?: HTMLElement): Promise<void>;
 
-  protected abstract afterFullRender(): Promise<void>;
-
   /**
    * Bootstrap main angular module with main component or send only new `props` with storyProps$
    *
@@ -144,18 +120,18 @@ export abstract class AbstractRenderer {
       analyzedMetadata,
     });
 
-    const applicationRef = await bootstrapApplication(application, {
-      ...storyFnAngular.applicationConfig,
-      providers: [
-        storyPropsProvider(newStoryProps$),
-        ...analyzedMetadata.applicationProviders,
-        ...(storyFnAngular.applicationConfig?.providers ?? []),
-      ],
+    const applicationRef = await queueBootstrapping(() => {
+      return bootstrapApplication(application, {
+        ...storyFnAngular.applicationConfig,
+        providers: [
+          storyPropsProvider(newStoryProps$),
+          ...analyzedMetadata.applicationProviders,
+          ...(storyFnAngular.applicationConfig?.providers ?? []),
+        ],
+      });
     });
 
     applicationRefs.set(targetDOMNode, applicationRef);
-
-    await this.afterFullRender();
   }
 
   /**

code/frameworks/angular/src/client/angular-beta/CanvasRenderer.ts

@@ -15,8 +15,4 @@ export class CanvasRenderer extends AbstractRenderer {
   async beforeFullRender(): Promise<void> {
     CanvasRenderer.resetApplications();
   }
-
-  async afterFullRender(): Promise<void> {
-    await AbstractRenderer.resetCompiledComponents();
-  }
 }

code/frameworks/angular/src/client/angular-beta/DocsRenderer.ts

@@ -44,10 +44,6 @@ export class DocsRenderer extends AbstractRenderer {
     DocsRenderer.resetApplications(domNode);
   }
 
-  async afterFullRender(): Promise<void> {
-    await AbstractRenderer.resetCompiledComponents();
-  }
-
   protected override initAngularRootElement(
     targetDOMNode: HTMLElement,
     targetSelector: string

code/frameworks/angular/src/client/angular-beta/RendererFactory.test.ts

@@ -224,5 +224,46 @@ describe('RendererFactory', () => {
         );
       });
     });
+
+    describe('when bootstrapping multiple stories in parallel', () => {
+      it('should render both stories', async () => {
+        @Component({ selector: 'foo', template: '🦊' })
+        class FooComponent {}
+
+        const render = await rendererFactory.getRendererInstance(
+          global.document.getElementById('storybook-docs')
+        );
+
+        const targetDOMNode1 = global.document.createElement('div');
+        targetDOMNode1.id = 'story-1';
+        global.document.getElementById('storybook-docs').appendChild(targetDOMNode1);
+
+        const targetDOMNode2 = global.document.createElement('div');
+        targetDOMNode2.id = 'story-2';
+        global.document.getElementById('storybook-docs').appendChild(targetDOMNode2);
+
+        await Promise.all([
+          render.render({
+            storyFnAngular: {},
+            forced: false,
+            component: FooComponent,
+            targetDOMNode: targetDOMNode1,
+          }),
+          render.render({
+            storyFnAngular: {},
+            forced: false,
+            component: FooComponent,
+            targetDOMNode: targetDOMNode2,
+          }),
+        ]);
+
+        expect(global.document.querySelector('#story-1 > story-1').innerHTML).toBe(
+          '<foo>🦊</foo><!--container-->'
+        );
+        expect(global.document.querySelector('#story-2 > story-2').innerHTML).toBe(
+          '<foo>🦊</foo><!--container-->'
+        );
+      });
+    });
   });
 });

code/frameworks/angular/src/client/angular-beta/StorybookWrapperComponent.ts

@@ -31,9 +31,6 @@ const getNonInputsOutputsProps = (
   return Object.keys(props).filter((k) => ![...inputs, ...outputs].includes(k));
 };
 
-// component modules cache
-export const componentNgModules = new Map<any, Type<any>>();
-
 /**
  * Wraps the story template into a component
  */
@@ -60,31 +57,20 @@ export const createStorybookWrapperComponent = ({
 
   const { imports, declarations, providers } = analyzedMetadata;
 
-  // Only create a new module if it doesn't already exist
-  // This is to prevent the module from being recreated on every story change
-  // Declarations & Imports are only added once
-  // Providers are added on every story change to allow for story-specific providers
-  let ngModule = componentNgModules.get(storyComponent);
-
-  if (!ngModule) {
-    @NgModule({
-      declarations,
-      imports,
-      exports: [...declarations, ...imports],
-    })
-    class StorybookComponentModule {}
-
-    componentNgModules.set(storyComponent, StorybookComponentModule);
-    ngModule = componentNgModules.get(storyComponent);
-  }
+  @NgModule({
+    declarations,
+    imports,
+    exports: [...declarations, ...imports],
+  })
+  class StorybookComponentModule {}
 
   PropertyExtractor.warnImportsModuleWithProviders(analyzedMetadata);
 
   @Component({
     selector,
     template,
     standalone: true,
-    imports: [ngModule],
+    imports: [StorybookComponentModule],
     providers,
     styles,
     schemas: moduleMetadata.schemas,

code/frameworks/angular/src/client/angular-beta/utils/BootstrapQueue.test.ts

@@ -0,0 +1,186 @@
+import { Subject, lastValueFrom } from 'rxjs';
+import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest';
+import assert from 'node:assert';
+
+import { queueBootstrapping } from './BootstrapQueue';
+
+describe('BootstrapQueue', () => {
+  beforeEach(async () => {
+    vi.spyOn(console, 'log').mockImplementation(() => {});
+  });
+
+  afterEach(() => {
+    vi.clearAllMocks();
+  });
+
+  it('should wait until complete', async () => {
+    const pendingSubject = new Subject<void>();
+    const bootstrapApp = vi.fn().mockImplementation(async () => {
+      return lastValueFrom(pendingSubject);
+    });
+    const bootstrapAppFinished = vi.fn();
+
+    queueBootstrapping(bootstrapApp).then(() => {
+      bootstrapAppFinished();
+    });
+
+    await vi.waitFor(() => {
+      assert(bootstrapApp.mock.calls.length === 1, 'bootstrapApp should have been called once');
+    });
+
+    expect(bootstrapApp).toHaveBeenCalled();
+    expect(bootstrapAppFinished).not.toHaveBeenCalled();
+
+    pendingSubject.next();
+    pendingSubject.complete();
+
+    await vi.waitFor(() => {
+      assert(
+        bootstrapAppFinished.mock.calls.length === 1,
+        'bootstrapApp should have been called once'
+      );
+    });
+
+    expect(bootstrapAppFinished).toHaveBeenCalled();
+  });
+
+  it('should prevent following tasks, until the preview tasks are complete', async () => {
+    const pendingSubject = new Subject<void>();
+    const bootstrapApp = vi.fn().mockImplementation(async () => {
+      return lastValueFrom(pendingSubject);
+    });
+    const bootstrapAppFinished = vi.fn();
+
+    const pendingSubject2 = new Subject<void>();
+    const bootstrapApp2 = vi.fn().mockImplementation(async () => {
+      return lastValueFrom(pendingSubject2);
+    });
+    const bootstrapAppFinished2 = vi.fn();
+
+    const pendingSubject3 = new Subject<void>();
+    const bootstrapApp3 = vi.fn().mockImplementation(async () => {
+      return lastValueFrom(pendingSubject3);
+    });
+    const bootstrapAppFinished3 = vi.fn();
+
+    queueBootstrapping(bootstrapApp).then(bootstrapAppFinished);
+    queueBootstrapping(bootstrapApp2).then(bootstrapAppFinished2);
+    queueBootstrapping(bootstrapApp3).then(bootstrapAppFinished3);
+
+    await vi.waitFor(() => {
+      assert(bootstrapApp.mock.calls.length === 1, 'bootstrapApp should have been called once');
+    });
+
+    expect(bootstrapApp).toHaveBeenCalled();
+    expect(bootstrapAppFinished).not.toHaveBeenCalled();
+    expect(bootstrapApp2).not.toHaveBeenCalled();
+    expect(bootstrapAppFinished2).not.toHaveBeenCalled();
+    expect(bootstrapApp3).not.toHaveBeenCalled();
+    expect(bootstrapAppFinished3).not.toHaveBeenCalled();
+
+    pendingSubject.next();
+    pendingSubject.complete();
+
+    await vi.waitFor(() => {
+      assert(bootstrapApp2.mock.calls.length === 1, 'bootstrapApp2 should have been called once');
+    });
+
+    expect(bootstrapApp).toHaveReturnedTimes(1);
+    expect(bootstrapAppFinished).toHaveBeenCalled();
+    expect(bootstrapApp2).toHaveBeenCalled();
+    expect(bootstrapAppFinished2).not.toHaveBeenCalled();
+    expect(bootstrapApp3).not.toHaveBeenCalled();
+    expect(bootstrapAppFinished3).not.toHaveBeenCalled();
+
+    pendingSubject2.next();
+    pendingSubject2.complete();
+
+    await vi.waitFor(() => {
+      assert(bootstrapApp3.mock.calls.length === 1, 'bootstrapApp3 should have been called once');
+    });
+
+    expect(bootstrapApp).toHaveReturnedTimes(1);
+    expect(bootstrapAppFinished).toHaveBeenCalled();
+    expect(bootstrapApp2).toHaveReturnedTimes(1);
+    expect(bootstrapAppFinished2).toHaveBeenCalled();
+    expect(bootstrapApp3).toHaveBeenCalled();
+    expect(bootstrapAppFinished3).not.toHaveBeenCalled();
+
+    pendingSubject3.next();
+    pendingSubject3.complete();
+
+    await vi.waitFor(() => {
+      assert(
+        bootstrapAppFinished3.mock.calls.length === 1,
+        'bootstrapAppFinished3 should have been called once'
+      );
+    });
+
+    expect(bootstrapApp).toHaveReturnedTimes(1);
+    expect(bootstrapAppFinished).toHaveBeenCalled();
+    expect(bootstrapApp2).toHaveReturnedTimes(1);
+    expect(bootstrapAppFinished2).toHaveBeenCalled();
+    expect(bootstrapApp3).toHaveReturnedTimes(1);
+    expect(bootstrapAppFinished3).toHaveBeenCalled();
+  });
+
+  it('should throw and continue next bootstrap on error', async () => {
+    const pendingSubject = new Subject<void>();
+    const bootstrapApp = vi.fn().mockImplementation(async () => {
+      return lastValueFrom(pendingSubject);
+    });
+    const bootstrapAppFinished = vi.fn();
+    const bootstrapAppError = vi.fn();
+
+    const pendingSubject2 = new Subject<void>();
+    const bootstrapApp2 = vi.fn().mockImplementation(async () => {
+      return lastValueFrom(pendingSubject2);
+    });
+    const bootstrapAppFinished2 = vi.fn();
+    const bootstrapAppError2 = vi.fn();
+
+    queueBootstrapping(bootstrapApp).then(bootstrapAppFinished).catch(bootstrapAppError);
+    queueBootstrapping(bootstrapApp2).then(bootstrapAppFinished2).catch(bootstrapAppError2);
+
+    await vi.waitFor(() => {
+      assert(bootstrapApp.mock.calls.length === 1, 'bootstrapApp should have been called once');
+    });
+
+    expect(bootstrapApp).toHaveBeenCalledTimes(1);
+    expect(bootstrapAppFinished).not.toHaveBeenCalled();
+    expect(bootstrapApp2).not.toHaveBeenCalled();
+
+    pendingSubject.error(new Error('test error'));
+
+    await vi.waitFor(() => {
+      assert(
+        bootstrapAppError.mock.calls.length === 1,
+        'bootstrapAppError should have been called once'
+      );
+    });
+
+    expect(bootstrapApp).toHaveBeenCalledTimes(1);
+    expect(bootstrapAppFinished).not.toHaveBeenCalled();
+    expect(bootstrapAppError).toHaveBeenCalledTimes(1);
+    expect(bootstrapApp2).toHaveBeenCalledTimes(1);
+    expect(bootstrapAppFinished2).not.toHaveBeenCalled();
+    expect(bootstrapAppError2).not.toHaveBeenCalled();
+
+    pendingSubject2.next();
+    pendingSubject2.complete();
+
+    await vi.waitFor(() => {
+      assert(
+        bootstrapAppFinished2.mock.calls.length === 1,
+        'bootstrapAppFinished2 should have been called once'
+      );
+    });
+
+    expect(bootstrapApp).toHaveBeenCalledTimes(1);
+    expect(bootstrapAppFinished).not.toHaveBeenCalled();
+    expect(bootstrapAppError).toHaveBeenCalledTimes(1);
+    expect(bootstrapApp2).toHaveBeenCalledTimes(1);
+    expect(bootstrapAppFinished2).toHaveBeenCalledTimes(1);
+    expect(bootstrapAppError2).not.toHaveBeenCalled();
+  });
+});