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: Remove cached NgModules and introduce a global queue during bootstrapping #24982

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9a7bb90
Remove cached NgModules and introduce a global lock during bootstrapping
Marklb Nov 25, 2023
1d42da9
Add tests and make bootstrapLock more generic
Marklb Nov 30, 2023
146178d
Merge branch 'next' into marklb/fix-multi-ng-app-rendering
Marklb Nov 30, 2023
3e338db
Merge remote-tracking branch 'origin/next' into pr/Marklb/24982
valentinpalkovic Dec 1, 2023
37e29e4
Use a queue instead of loops and timeouts
valentinpalkovic Dec 1, 2023
d14ad85
Reset compiled components after bootstrapping
valentinpalkovic Dec 1, 2023
043637c
Merge branch 'next' into marklb/fix-multi-ng-app-rendering
Marklb Dec 2, 2023
f729eb8
Update test and names to refer to "queue" instead of "lock"
Marklb Dec 2, 2023
29ab192
Reset compiled components before bootstraping
valentinpalkovic Dec 4, 2023
5600238
Merge branch 'next' into marklb/fix-multi-ng-app-rendering
valentinpalkovic Dec 6, 2023
326c9c0
Merge branch 'next' into marklb/fix-multi-ng-app-rendering
valentinpalkovic Dec 11, 2023
ece4613
Merge branch 'next' into marklb/fix-multi-ng-app-rendering
Marklb Dec 12, 2023
ce32d79
Merge branch 'next' into marklb/fix-multi-ng-app-rendering
Marklb Dec 12, 2023
32c4019
Merge branch 'next' into marklb/fix-multi-ng-app-rendering
valentinpalkovic Dec 19, 2023
cd968bb
Merge branch 'next' into marklb/fix-multi-ng-app-rendering
Marklb Dec 28, 2023
f71ca84
Migrate tests to vitest
Marklb Dec 28, 2023
a4ebf57
Merge branch 'next' into marklb/fix-multi-ng-app-rendering
valentinpalkovic Jan 15, 2024
f7d5d79
Merge remote-tracking branch 'origin/next' into pr/Marklb/24982
valentinpalkovic Jan 16, 2024
e94ad41
Fix flaky Angular tests
valentinpalkovic Jan 17, 2024
793cea6
Refactor BootstrapQueue.ts to improve code execution order
valentinpalkovic Jan 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 11 additions & 35 deletions code/frameworks/angular/src/client/angular-beta/AbstractRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ import { bootstrapApplication } from '@angular/platform-browser';

import { BehaviorSubject, Subject } from 'rxjs';
import { stringify } from 'telejson';
import { ICollection, Parameters, StoryFnAngularReturnType } from '../types';
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/BootstrapLock';

type StoryRenderInfo = {
storyFnAngular: StoryFnAngularReturnType;
Expand All @@ -21,35 +21,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
Expand All @@ -69,8 +47,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$
*
Expand Down Expand Up @@ -129,18 +105,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();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,4 @@ export class CanvasRenderer extends AbstractRenderer {
async beforeFullRender(): Promise<void> {
CanvasRenderer.resetApplications();
}

async afterFullRender(): Promise<void> {
await AbstractRenderer.resetCompiledComponents();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,4 @@ export class DocsRenderer extends AbstractRenderer {
async beforeFullRender(domNode?: HTMLElement): Promise<void> {
DocsRenderer.resetApplications(domNode);
}

async afterFullRender(): Promise<void> {
await AbstractRenderer.resetCompiledComponents();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ describe('RendererFactory', () => {
beforeEach(async () => {
rendererFactory = new RendererFactory();
document.body.innerHTML =
'<div id="storybook-root"></div><div id="root-docs"><div id="story-in-docs"></div></div>';
'<div id="storybook-root"></div><div id="root-docs"><div id="story-in-docs"></div></div>' +
'<div id="storybook-docs"></div>';
rootTargetDOMNode = global.document.getElementById('storybook-root');
rootDocstargetDOMNode = global.document.getElementById('root-docs');
(platformBrowserDynamic as any).mockImplementation(platformBrowserDynamicTesting);
Expand Down Expand Up @@ -180,5 +181,46 @@ describe('RendererFactory', () => {
const render = await rendererFactory.getRendererInstance(rootDocstargetDOMNode);
expect(render).toBeInstanceOf(DocsRenderer);
});

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-->'
);
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import { Subject } from 'rxjs';

import { queueBootstrapping } from './BootstrapLock';

const tick = (count = 0) => new Promise((resolve) => setTimeout(resolve, count));

describe('BootstrapLock', () => {
beforeEach(async () => {
jest.spyOn(console, 'log').mockImplementation(() => {});
});

afterEach(() => {
jest.clearAllMocks();
});

it('should lock until complete', async () => {
const pendingSubject = new Subject<void>();
const bootstrapApp = jest.fn().mockImplementation(async () => {
return pendingSubject.toPromise();
});
const bootstrapAppFinished = jest.fn();

queueBootstrapping(bootstrapApp).then(() => {
bootstrapAppFinished();
});

expect(bootstrapApp).toHaveBeenCalled();
expect(bootstrapAppFinished).not.toHaveBeenCalled();

pendingSubject.next();
pendingSubject.complete();

await tick();

expect(bootstrapAppFinished).toHaveBeenCalled();
});

it('should prevent following tasks, until the preview tasks are complete', async () => {
const pendingSubject = new Subject<void>();
const bootstrapApp = jest.fn().mockImplementation(async () => {
return pendingSubject.toPromise();
});
const bootstrapAppFinished = jest.fn();

const pendingSubject2 = new Subject<void>();
const bootstrapApp2 = jest.fn().mockImplementation(async () => {
return pendingSubject2.toPromise();
});
const bootstrapAppFinished2 = jest.fn();

const pendingSubject3 = new Subject<void>();
const bootstrapApp3 = jest.fn().mockImplementation(async () => {
return pendingSubject3.toPromise();
});
const bootstrapAppFinished3 = jest.fn();

queueBootstrapping(bootstrapApp).then(bootstrapAppFinished);
queueBootstrapping(bootstrapApp2).then(bootstrapAppFinished2);
queueBootstrapping(bootstrapApp3).then(bootstrapAppFinished3);

await tick();

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 tick();

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 tick();

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 tick();

expect(bootstrapApp).toHaveReturnedTimes(1);
expect(bootstrapAppFinished).toHaveBeenCalled();
expect(bootstrapApp2).toHaveReturnedTimes(1);
expect(bootstrapAppFinished2).toHaveBeenCalled();
expect(bootstrapApp3).toHaveReturnedTimes(1);
expect(bootstrapAppFinished3).toHaveBeenCalled();
});

it('should reset lock on error', async () => {
const pendingSubject = new Subject<void>();
const bootstrapApp = jest.fn().mockImplementation(async () => {
return pendingSubject.toPromise();
});
const bootstrapAppFinished = jest.fn();
const bootstrapAppError = jest.fn();

queueBootstrapping(bootstrapApp).then(bootstrapAppFinished).catch(bootstrapAppError);

expect(bootstrapApp).toHaveBeenCalled();
expect(bootstrapAppFinished).not.toHaveBeenCalled();

// eslint-disable-next-line local-rules/no-uncategorized-errors
pendingSubject.error(new Error('test error'));

await tick();

expect(bootstrapApp).toHaveReturnedTimes(1);
expect(bootstrapAppFinished).not.toHaveBeenCalled();
expect(bootstrapAppError).toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/* eslint-disable no-await-in-loop */
import { ApplicationRef } from '@angular/core';

const queue: Array<() => Promise<void>> = [];
let isProcessing = false;

/**
* Reset compiled components because we often want to compile the same component with
* more than one NgModule.
*/
const 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
*/
}
};

export const queueBootstrapping = (fn: () => Promise<ApplicationRef>): Promise<ApplicationRef> => {
return new Promise<ApplicationRef>((resolve, reject) => {
queue.push(() => fn().then(resolve).catch(reject));

if (!isProcessing) {
processQueue();
}
});
};

const processQueue = async () => {
isProcessing = true;

while (queue.length > 0) {
const bootstrappingFn = queue.shift();
if (bootstrappingFn) {
await bootstrappingFn();
await resetCompiledComponents();
Marklb marked this conversation as resolved.
Show resolved Hide resolved
Marklb marked this conversation as resolved.
Show resolved Hide resolved
}
}

isProcessing = false;
};