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: Add support for angular 13.1 #16978

Closed
wants to merge 4 commits into from

Conversation

leon
Copy link

@leon leon commented Dec 10, 2021

Issue: #16977

What I did

Angular 13.1 changed the method name of getTypescriptWorkerPlugin to getDevServerConfig
which caused storybook to fail with getTypescriptWorkerPlugin method is undefined.

I saw we have specific code for certain versions of Angular cli, so I simply copied this abstraction for angular 13.1

getTypescriptWorkerPlugin changed names to getDevServerConfig

storybookjs#16977
@nx-cloud
Copy link

nx-cloud bot commented Dec 10, 2021

Nx Cloud Report

CI ran the following commands for commit 44f78da. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@shilman shilman changed the title fix: support angular 13.1 Angular: Add support for angular 13.1 Dec 10, 2021
@leon
Copy link
Author

leon commented Dec 11, 2021

Since this is something that works for Storybook 6.4 also, should I create a pull request for 6.4 also?
I don't want to have to wait for 6.5 to get released.

@literalpie
Copy link
Contributor

literalpie commented Dec 11, 2021

I'm not sure I'm convinced that getDevServerConfig is a replacement for getTypescriptWorkerPlugin. They seem to do very different things.

I'm looking at this change (where getTSWorkerPlugin was removed) and it seems to me like the createIvyPlugin (which used to be the only thing getTypescriptWorkerPlugin would return) is now included in getCommonConfig. Maybe getTypescriptWorkerPlugin can just be removed?

(This is all based on reading the code. I haven't done testing yet)

@leon
Copy link
Author

leon commented Dec 11, 2021

I tried removing the call to getTypescriptWorkerPlugin completely and it seems to be working.

But I need some help testing and reviewing, since this is my first dive into storybook, I don't feel confident saying its fixed.

@literalpie
Copy link
Contributor

I also tried making this change, but I'm not seeing success when installing the package into the minimal repro of this issue. I get

Error: /{redacted}/storybook-repros/sb-angular13.1/src/stories/Button.stories.ts is missing from the TypeScript compilation. Please make sure it is in your tsconfig via the 'files' or 'include' property.

Looking more at the Angular code change that caused this issue here, I think it's good that they added the IvyPlugin, but bad that they added the angular webpack loader for all files that match those paths.

Changing line 61 of we angular-cli-13.x file to this fixed it for me:

    rules: [
      // In angular 13.1, the angular webpack loader gets added for all ts files.
      // We don't want this since it will try and fail to analyze story files.
      ...cliConfig.module.rules.filter((rule) => !rule.loader?.includes('@ngtools/webpack')),
      ...rulesExcludingStyles,
    ],

This is a very "proof of concept" solution since I think there may be valid reasons for the @ngtools/webpack loader to be used, but I confess I don't know what those reasons would be. There's a lot about Angular CLI I don't really understand.

Sorry, I don't have time to research and answer some of the questions I still have, but hopefully this helps.

@leon
Copy link
Author

leon commented Dec 14, 2021

There seems to be more complex problems, such as errors saying files are missing from the compilation.
I'm not experienced enough with the inner workings of either storybook or the angular cli, so I'm going to let someone else give it a go :)

@leon leon closed this Dec 14, 2021
@ThibaudAV
Copy link
Contributor

I've also started looking.
First, It works for some stories on a project but not all 🤷‍♂️
I think there is a problem with the preset framework-preset-angular. like that, I would say that it should not be necessary for angular 13+ anymore 🤔
So i've removed in my node_modules/@storybook/angular/dist/ts3.9/server/preset.js and it seems to work better

Another person could also make the test and tell me if it works? 🙏 🙏 🙏 if yes I could make the necessary to be also conditioned with angular version

@literalpie
Copy link
Contributor

Sorry for the flip-flopping, but when I tried removing getTypescriptWorkerPlugin again today, it is working fine. here is a branch with that patch included using yarn. Hopefully I'll be able to reproduce my error from last week again so we can track down when it is/isn't a problem.

If I also remove framework-preset-angular, I get this error:

ModuleParseError: Module parse failed: Unexpected token (11:2)
File was processed with these loaders:
 * ./node_modules/@angular-devkit/build-angular/src/babel/webpack-loader.js
 * ./node_modules/@storybook/source-loader/dist/cjs/index.js
You may need an additional loader to handle the result of these loaders.
|     backgroundColor: { control: "color" },
|   },
> } as Meta;
| 
| const Template: Story<Button> = (args: Button) => ({
    at handleParseError (/Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/NormalModule.js:941:19)
    at /Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/NormalModule.js:1045:5
    at processResult (/Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/NormalModule.js:763:11)
    at /Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/NormalModule.js:827:5
    at /Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/loader-runner/lib/LoaderRunner.js:406:3
    at iterateNormalLoaders (/Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/loader-runner/lib/LoaderRunner.js:232:10)
    at iterateNormalLoaders (/Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/loader-runner/lib/LoaderRunner.js:239:10)
    at /Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/loader-runner/lib/LoaderRunner.js:254:3
    at context.callback (/Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/loader-runner/lib/LoaderRunner.js:124:13)
    at /Users/bkindle/Documents/Development/temp/sb-ng-performance/node_modules/@angular-devkit/build-angular/node_modules/babel-loader/lib/index.js:59:71

On my large project, removing getTypescriptWorkerPlugin also causes storybook to build successfully! But it seems like certain component styles aren't getting loaded properly, so I will have to look into that.

@literalpie
Copy link
Contributor

literalpie commented Dec 18, 2021

See here for a new pull request which replaced this one.

Also, to follow up on my "missing from the TypeScript compilation" issue mentioned above, I created this issue. I was able to narrow down the minimal reproduction of that error.

@roshan2921
Copy link

roshan2921 commented Jan 13, 2022

No it's not working for me I'm also getting same error@literalpie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants