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 all 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
44 changes: 10 additions & 34 deletions code/frameworks/angular/src/client/angular-beta/AbstractRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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$
*
Expand Down Expand Up @@ -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();
}

/**
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 @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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-->'
);
});
});
});
});
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,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();
});
});
Loading