-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Conversation
If you start the example using this configuration all you get is
for every *.ts file in the example/angular-cli directory. |
@kroeder I've created a tech demo showing what needs to be done to make CSF stories AoT compatible. I don't know how to integrate it with storybook, but its a start: https://github.com/stupidawesome/storybook-csf-angular-aot-demo If this is the approach we take there needs to be discussion on what syntax should be supported for writing stories since it has to be statically analyzable to work. |
@stupidawesome can you elaborate on statically analyzable? I’m familiar with what that means in general from a compiler standpoint, but what does it mean specifically for AoT? Reference? EDIT: checked out your repo and the examples look reasonable to me. I’m pretty busy with 6.0 release right now but when that’s further along I’d love to go through this together and help get it integrated! |
What I mean by statically analyzable is that which can be read/transformed/extracted by traversing the AST of the source files for each story. In practice it means that we expect identifiers and expressions to be in a certain format and location so that the parser can apply the right transforms to generate AoT compatible code (eg. arrow function/function declaration, parenthesized expression/return statement, top level variable statements etc). Anything outside of what is specified will either produce unexpected behavior,or simply fail to compile. For example: creating stories inside loop statements or arbitrary closures. The requirements for making each story AoT compatible are:
Each story could then be compiled as a self contained microapp and served through the storybook canvas iframe. However |
@stupidawesome I finally could take a look at your repo. Amazing job! Thank you for that, it helped me a lot. 🙂 @shilman and I are going to chat about this on Thursday, 8AM (UTC) Either reply here or join our discord https://discord.com/invite/UUt2PJb if you are interested 🙂 |
@kroeder Can you make it 8:30? I've got a meeting to go to. |
@stupidawesome sure! Can you join our discord so I can send you the zoom link later? https://discord.com/invite/UUt2PJb |
It looks like we can enable Ivy while keeping things JIT with a few tweaks.
I think we should explore a way to compile user components with AoT separately while keeping the storybook metadata dynamic with JIT. That will hopefully avoid the need to do complicated TypeScript transforms while gaining the benefits of template type checking and dynamic storybook decorators. |
@stupidawesome I pushed my changes
But it should be part of the tsconfig file 🤔 |
Is this still being worked on? Soon one of our angular components will not be usable for us anymore because it's only published with AOT required. |
@kossmoboleat I am attempting to make this or something more like the cli builders, but I haven't got something that would really work yet. Since I had only used builders that hook into the cli for inserting webpack customizations, I have been looking into them and breaking down Angular's building code to try and understand enough to decide if I am on Discord a lot, if anyone is working on this and wants to discuss it or has any questions I may can answer about Storybook's code. |
@Marklb have you tried to discuss this stuff with angular/nrwl stuff members? You can find some of them in angular discord https://discord.gg/angular and maybe have some brainstorm to understand how to integrate aot build to stroybook. I think a lot of people there are interested in this |
@artaommahe No, I haven't. I periodically go through their code or issues to check on questions people ask about Storybook, but haven't really discussed this with anyone. Now that I have a better understanding of the process, I may try to reach out to someone. As for the Angular Discord, I watch the Storybook channel there and mentioned updating Storybook to support Ivy a few times, without any feedback. It wasn't necessarily questions I asked, so it isn't like I was ignored, just general comments that I am looking into it and would try to answer questions if anyone with more knowledge is looking into it. |
@Marklb mb point directly that you are trying to integrate aot stuff to storybook and need help? Also tag some people from angular/nrwl stuff and ask them to discuss this case or to get contacts to talk with, mb someone who works with angular-cli. Usually direct mentions work better than generic question |
@Marklb just remembered, as far as i understand guys at jest-preset-angular have already done such integration of ivy thymikee/jest-preset-angular#409. Mb their solution can be applied here or to discuss this stuff with them |
@Marklb @artaommahe |
Reminder: Include @angular/forms and @ngrx/* as ngcc target (internally) |
import options from './options'; | ||
|
||
runNgcc(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is where it is coming from?
There was a problem hiding this comment.
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;
@@ -205,7 +205,7 @@ | |||
"ts-dedent": "^2.0.0", | |||
"ts-jest": "^26.4.4", | |||
"ts-node": "^8.10.2", | |||
"typescript": "^3.9.7", | |||
"typescript": "^4.1.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool then we can proceed, with the upgrade I think!
@kroeder looks like some TS errors came up since the upgrade? |
Great work @kroeder ❤️ Let's merge this at the start of 6.3 so that we have lots of time to stabilize in case there are issues with Ivy, Typescript version, etc. |
# Conflicts: # addons/storyshots/storyshots-core/package.json # yarn.lock
I resolved merge cnflicts and synced the typescript version (cc @gaetanmaisse) There's a bunch of test failing in the angular app @kroeder:
|
I tried testing this by installing it with the commit id directly:
Unfortunately looks like it's not as easy, I guess it needs a build step? Is there a version (tag?) published of this PR? |
@Rush Yes it is
not to my knowledge in some cases to test I use If you want to try |
# Conflicts: # addons/storyshots/storyshots-core/package.json # yarn.lock
running into this:
|
I have no clue why tests start to fail now. Those token errors are usually thrown when something is wrong in the compiler configuration 🤔 I haven't even changed anything in any tsconfig Maybe this is caused by the typescript upgrade |
I tried to rollback to typescript 3.9 but got the exact same error. I tried |
@@ -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', |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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
# Conflicts: # addons/storyshots/storyshots-core/package.json
Closing this for now. AFAIK this is being actively developed in https://github.com/storybookjs/addon-angular-ivy/. When that's ready we can bring that back into core. And if there is useful work in this PR, we can always restore it later. |
Issue: #10580 #10760 #10863 #13748 #13768
What I did
After the meeting with the Angular Team we figured out the missing piece of information that enables ivy support.
If this is an environment without Ivy then it behaves the same as Storybook did before.
ngcc
@angular/platform-browser-dynamic
must be converted usingngcc
ngcc
post-install
or during the startup of an Angular applicationHow to test
tba