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

WIP: feat(angular): add ivy support with JIT #11157

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
2 changes: 1 addition & 1 deletion addons/docs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@
"util-deprecate": "^1.0.2"
},
"devDependencies": {
"@angular/core": "^11.2.0",
"@angular/core": "^11.2.3",
"@babel/core": "^7.12.10",
"@emotion/core": "^10.1.1",
"@emotion/styled": "^10.0.27",
Expand Down
4 changes: 2 additions & 2 deletions addons/storyshots/storyshots-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@
"ts-dedent": "^2.0.0"
},
"devDependencies": {
"@angular/core": "^11.2.0",
"@angular/platform-browser-dynamic": "^11.2.0",
"@angular/core": "^11.2.3",
"@angular/platform-browser-dynamic": "^11.2.3",
"@storybook/addon-docs": "6.2.0-rc.4",
"@storybook/angular": "6.2.0-rc.4",
"@storybook/react": "6.2.0-rc.4",
Expand Down
3 changes: 3 additions & 0 deletions app/angular/jest.config.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
module.exports = {
preset: 'jest-preset-angular',
setupFilesAfterEnv: ['<rootDir>/setup-jest.ts'],
moduleNameMapper: {
'@storybook/node-logger': '<rootDir>/../../lib/node-logger/dist/cjs/index.js',
Copy link
Member Author

@kroeder kroeder Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't know how jest managed to successfully run tests with external packages without moduleNameMapper before 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now build-storybook fails with

ERR! Module parse failed: Unexpected character '@' (11:61)

I guess that's not the solution we were looking for

},
};
20 changes: 10 additions & 10 deletions app/angular/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,16 +67,16 @@
"webpack": "4"
},
"devDependencies": {
"@angular-devkit/build-angular": "~0.1102.0",
"@angular-devkit/core": "^11.2.0",
"@angular/common": "^11.2.0",
"@angular/compiler": "^11.2.0",
"@angular/compiler-cli": "^11.2.0",
"@angular/core": "^11.2.0",
"@angular/elements": "^11.2.0",
"@angular/forms": "^11.2.0",
"@angular/platform-browser": "^11.2.0",
"@angular/platform-browser-dynamic": "^11.2.0",
"@angular-devkit/build-angular": "~0.1102.2",
"@angular-devkit/core": "^11.2.2",
"@angular/common": "^11.2.3",
"@angular/compiler": "^11.2.3",
"@angular/compiler-cli": "^11.2.3",
"@angular/core": "^11.2.3",
"@angular/elements": "^11.2.3",
"@angular/forms": "^11.2.3",
"@angular/platform-browser": "^11.2.3",
"@angular/platform-browser-dynamic": "^11.2.3",
"@nrwl/workspace": "^11.1.5",
"@types/autoprefixer": "^9.7.2",
"@types/jest": "^26.0.16",
Expand Down
19 changes: 18 additions & 1 deletion app/angular/src/client/preview/angular-beta/RendererService.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable no-undef */
import { enableProdMode, PlatformRef } from '@angular/core';
import { enableProdMode, PlatformRef, ɵresetCompiledComponents } from '@angular/core';
import { platformBrowserDynamic } from '@angular/platform-browser-dynamic';

import { BehaviorSubject, Subject } from 'rxjs';
Expand Down Expand Up @@ -74,6 +74,23 @@ export class RendererService {
}
this.storyProps$ = new BehaviorSubject<ICollection>(storyFnAngular.props);

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
ɵresetCompiledComponents();
} catch (e) {
/**
* noop catch
* This means angular removed or modified ɵresetCompiledComponents
*
* Prorably, they added a clearCache mechanism to platform.destroy() and
* we can simply remove this in case no errors are thrown during runtime
*/
}

await this.newPlatformBrowserDynamic().bootstrapModule(
createStorybookModule(
getStorybookModuleMetadata({ storyFnAngular, parameters }, this.storyProps$)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ export const createStorybookWrapperComponent = (
styles: string[],
initialProps?: ICollection
): Type<any> => {
// In ivy, a '' selector is not allowed, therefore we need to just set it to anything if
// storyComponent was not provided.
const viewChildSelector = storyComponent ?? '__storybook-noop';

@Component({
selector: RendererService.SELECTOR_STORYBOOK_WRAPPER,
template,
Expand All @@ -52,9 +56,9 @@ export const createStorybookWrapperComponent = (

private storyWrapperPropsSubscription: Subscription;

@ViewChild(storyComponent ?? '', { static: true }) storyComponentElementRef: ElementRef;
@ViewChild(viewChildSelector, { static: true }) storyComponentElementRef: ElementRef;

@ViewChild(storyComponent ?? '', { read: ViewContainerRef, static: true })
@ViewChild(viewChildSelector, { read: ViewContainerRef, static: true })
storyComponentViewContainerRef: ViewContainerRef;

// Used in case of a component without selector
Expand Down
2 changes: 2 additions & 0 deletions app/angular/src/server/build.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { buildStatic } from '@storybook/core/server';
import { runNgcc } from './ngcc-execution';
import options from './options';

runNgcc();
buildStatic(options);
9 changes: 9 additions & 0 deletions app/angular/src/server/framework-preset-angular.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,15 @@ export function webpack(
},
resolve: {
...config.resolve,
mainFields: [
'es2015_ivy_ngcc',
'module_ivy_ngcc',
'main__ivy_ngcc',
'es2015',
'browser',
'module',
'main',
],
},
plugins: [
...config.plugins,
Expand Down
2 changes: 2 additions & 0 deletions app/angular/src/server/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { buildDev } from '@storybook/core/server';
import { runNgcc } from './ngcc-execution';
import options from './options';

runNgcc();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a synchronous operation, correct?

Copy link
Member Author

@kroeder kroeder Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can choose https://github.com/angular/angular/blob/master/packages/compiler-cli/ngcc/index.ts#L17

If I would provide AsyncNgccOptions as generic it would be async but as far as I can see it should be sync
I tested it in an app yesterday and it definitely was 🙂

Copy link

@petebacondarwin petebacondarwin Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be aware that, in sync mode, ngcc will crash out if there is another ngcc process running at the same time - it is not able to wait like it can in async mode. This is probably not a problem but worth being aware of.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info! I can imagine that someone starts the app + storybook in parallel right after installing all dependencies
Using async should cause no further issues, right? We just need to await ngcc

If it is that simple to prevent a crash then let's do this 🙂

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can call runNgcc() asynchronously then it is probably safest to do so.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That error indicates that you are somehow triggering ngcc via its command line executable.
I don't suppose you have a prestorybook or prebuild script or something that is still running ngcc from the command line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, only from the code. I extracted the code from this PR in a smaller external package which makes it easier to see what happens: https://github.com/storybookjs/addon-angular-ivy/blob/main/src/preset/index.ts

That's all

There are 2 ways to avoid the error

  • Use async: false
  • Use async: true and don't add arguments to the "start" script

No other prebuild steps involved: https://github.com/storybookjs/addon-angular-ivy/blob/main/package.json#L22

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just realised... in async mode we kick off child processes, which will receive the arguments passed to the original parent process. Perhaps this is how they are getting passed to ngcc. I'll investigate

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A workaround would be to clear the command line args before triggering ngcc:

process.argv.length = 0;

buildDev(options);
16 changes: 16 additions & 0 deletions app/angular/src/server/ngcc-execution.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import path from 'path';
import { process as ngccProcess } from '@angular/compiler-cli/ngcc';

/**
* Run ngcc for converting modules to ivy format before starting storybook
* This step is needed in order to support Ivy in storybook
*
* Information about Ivy can be found here https://angular.io/guide/ivy
*/
export function runNgcc() {
ngccProcess({
basePath: path.join(process.cwd(), 'node_modules'), // absolute path to node_modules
createNewEntryPointFormats: true, // --create-ivy-entry-points
compileAllFormats: false, // --first-only
});
}
2 changes: 1 addition & 1 deletion app/aurelia/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
"rimraf": "^3.0.2",
"sass-loader": "^8.0.2",
"style-loader": "^0.23.0",
"typescript": "^3.9.7",
"typescript": "^4.1.3",
"webpack": "^4.46.0"
},
"peerDependencies": {
Expand Down
10 changes: 6 additions & 4 deletions app/react/src/client/preview/render.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import { document, FRAMEWORK_OPTIONS } from 'global';
import React, { Component, FunctionComponent, ReactElement, StrictMode, Fragment } from 'react';
import React, { Component, FunctionComponent, StrictMode, Fragment } from 'react';
import ReactDOM from 'react-dom';

import { StoryContext, RenderContext } from './types';

const rootEl = document ? document.getElementById('root') : null;

const render = (node: ReactElement, el: Element) =>
new Promise((resolve) => {
type RenderArgs = Parameters<typeof ReactDOM.render>;

const render = (node: RenderArgs[0], el: RenderArgs[1]) =>
new Promise<void>((resolve) => {
ReactDOM.render(node, el, resolve);
});

Expand Down Expand Up @@ -72,5 +74,5 @@ export default async function renderMain({
ReactDOM.unmountComponentAtNode(rootEl);
}

await render(element, rootEl);
await render([element], rootEl);
}
22 changes: 11 additions & 11 deletions examples/angular-cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,24 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@angular/common": "^11.2.0",
"@angular/compiler": "^11.2.0",
"@angular/core": "^11.2.0",
"@angular/forms": "^11.2.0",
"@angular/platform-browser": "^11.2.0",
"@angular/platform-browser-dynamic": "^11.2.0",
"@angular/common": "^11.2.3",
"@angular/compiler": "^11.2.3",
"@angular/core": "^11.2.3",
"@angular/forms": "^11.2.3",
"@angular/platform-browser": "^11.2.3",
"@angular/platform-browser-dynamic": "^11.2.3",
"@ngrx/store": "^10.1.2",
"core-js": "^3.8.2",
"node-sass": "^4.14.1",
"rxjs": "^6.6.3",
"zone.js": "^0.11.3"
},
"devDependencies": {
"@angular-devkit/build-angular": "~0.1102.0",
"@angular-devkit/core": "^11.2.0",
"@angular/cli": "^11.2.0",
"@angular/compiler-cli": "^11.2.0",
"@angular/elements": "^11.2.0",
"@angular-devkit/build-angular": "~0.1102.2",
"@angular-devkit/core": "^11.2.3",
"@angular/cli": "^11.2.3",
"@angular/compiler-cli": "^11.2.3",
"@angular/elements": "^11.2.3",
"@compodoc/compodoc": "^1.1.11",
"@storybook/addon-a11y": "6.2.0-rc.4",
"@storybook/addon-actions": "6.2.0-rc.4",
Expand Down
2 changes: 1 addition & 1 deletion examples/aurelia-kitchen-sink/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"sass-loader": "^8.0.2",
"style-loader": "^0.23.0",
"ts-loader": "^6.2.2",
"typescript": "^3.9.7",
"typescript": "^4.1.3",
"webpack": "^4.46.0"
}
}
2 changes: 1 addition & 1 deletion examples/cra-ts-essentials/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
"react": "16.14.0",
"react-dom": "16.14.0",
"react-scripts": "^4.0.2",
"typescript": "^3.9.7"
"typescript": "^4.1.3"
},
"devDependencies": {
"@storybook/addon-essentials": "6.2.0-rc.4",
Expand Down
2 changes: 1 addition & 1 deletion examples/cra-ts-kitchen-sink/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
"react": "16.14.0",
"react-dom": "16.14.0",
"react-scripts": "^4.0.2",
"typescript": "^3.9.7"
"typescript": "^4.1.3"
},
"devDependencies": {
"@storybook/addon-a11y": "6.2.0-rc.4",
Expand Down
2 changes: 1 addition & 1 deletion examples/react-ts-webpack4/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"prop-types": "15.7.2",
"react": "16.14.0",
"react-dom": "16.14.0",
"typescript": "^3.9.7",
"typescript": "^4.1.3",
"webpack": "4"
}
}
2 changes: 1 addition & 1 deletion examples/react-ts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"prop-types": "15.7.2",
"react": "16.14.0",
"react-dom": "16.14.0",
"typescript": "^3.9.7",
"typescript": "^4.1.3",
"webpack": "4"
}
}
2 changes: 1 addition & 1 deletion examples/vue-cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"@vue/cli-plugin-babel": "~4.3.1",
"@vue/cli-plugin-typescript": "~4.3.1",
"@vue/cli-service": "~4.3.1",
"typescript": "^3.9.7",
"typescript": "^4.1.3",
"vue-template-compiler": "^2.6.12"
}
}
Loading