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

Build: Fix Angular sandbox #23896

Merged

Conversation

Marklb
Copy link
Member

@Marklb Marklb commented Aug 21, 2023

Closes #

What I did

Fix errors preventing Angular sandboxes from running.

I regenerated the lock file, to fix an issue with browserlist, I think. The error was Unknown version 115 of edge (While processing: "base$0$0").

The linked dev lib builds do not work with the default Angular tsconfig, so I set:

"compilerOptions": {
    ...,
    "noImplicitOverride": false,
    "noPropertyAccessFromIndexSignature": false,
    "jsx": "react"
}

I did not narrow down what import is causing the node_modules/@storybook/angular/node_modules/@angular-devkit to get used, instead of node_modules/@angular-devkit, but for the Angular 15 sandbox I set a path in the tsconfig:

"compilerOptions": {
    ...,
    "paths": {
      "@angular-devkit/*": [
        "node_modules/@angular-devkit/*"
      ]
    }
}

How to test

  1. Run the three sandbox templates for dev, e.g. yarn task --task dev --template angular-cli/15-ts, yarn task --task dev --template angular-cli/default-ts, yarn task --task dev --template angular-cli/prerelease

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@Marklb Marklb added bug angular build Internal-facing build tooling & test updates labels Aug 21, 2023
@socket-security
Copy link

socket-security bot commented Aug 21, 2023

Updated and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@storybook/addon-designs 7.0.5...7.0.7 None +0/-2 113 kB
@storybook/icons 1.2.2...1.2.3 None +0/-0 1.16 MB ndelangen
svelte 4.2.7...4.2.8 None +0/-0 2.63 MB svelte-admin
@vitejs/plugin-vue 4.5.0...4.5.1 None +0/-1 183 kB vitebot

🚮 Removed packages: @storybook/client-logger@7.6.0, @storybook/theming@7.6.0

@JReinhold
Copy link
Contributor

Thanks for the PR @Marklb ! 💪

How would I reproduce the issue that you're fixing here?

@Marklb
Copy link
Member Author

Marklb commented Aug 21, 2023

Thanks for the PR @Marklb ! 💪

How would I reproduce the issue that you're fixing here?

Just cloning the repo, without these changes, and running any of the three Angular sandboxes has been failing for a while.

@Marklb
Copy link
Member Author

Marklb commented Aug 31, 2023

I will update this to the current next branch in a little bit, but is there anything else I need to do or do differently?

This only happens when using the dev build of the libraries, so I am applying them and keeping them unstaged every time I want to try any changes in an Angular sandbox.

I can maybe add a condition to only apply the changes when generating a dev sandbox, if that is exposed to the sandbox generation, or do the libraries dev builds need to be changed? I don't remember which libraries were causing the issues, but I can try to find those instead of adjusting the sandbox.

@yannbf
Copy link
Member

yannbf commented Sep 1, 2023

I will update this to the current next branch in a little bit, but is there anything else I need to do or do differently?

This only happens when using the dev build of the libraries, so I am applying them and keeping them unstaged every time I want to try any changes in an Angular sandbox.

I can maybe add a condition to only apply the changes when generating a dev sandbox, if that is exposed to the sandbox generation, or do the libraries dev builds need to be changed? I don't remember which libraries were causing the issues, but I can try to find those instead of adjusting the sandbox.

Hey Mark! Sorry we haven't looked into this yet. I'll do this as first thing next week. Thank you so much!!

@yannbf
Copy link
Member

yannbf commented Sep 4, 2023

Hey @Marklb I updated the branch with the latest changes in next and when I create an angular sandbox (angular-cli-default-ts) the creation is successful, however when running the sandbox I get many issues:

1:

info => Starting manager..
info => Starting preview..
info Addon-docs: using MDX2
info => Using implicit CSS loaders
info => Using angular browser target options from "angular-latest:build"
info => Using angular project with "tsConfig:/sandbox/angular-cli-default-ts/.storybook/tsconfig.json"
info => Using default Webpack5 setup
<i> [webpack-dev-middleware] wait until bundle finished
ERROR in ./src/styles.scss?ngGlobalStyle
Module build failed (from ../../code/frameworks/angular/node_modules/@angular-devkit/build-angular/node_modules/mini-css-extract-plugin/dist/loader.js):
Error: You forgot to add 'mini-css-extract-plugin' plugin (i.e. `{ plugins: [new MiniCssExtractPlugin()] }`), please read https://github.com/webpack-contrib/mini-css-extract-plugin#getting-started
    at Object.pitch (/Users/yannbraga/open-source/storybook/code/frameworks/angular/node_modules/@angular-devkit/build-angular/node_modules/mini-css-extract-plugin/dist/loader.js:77:14)

2:

ERROR in ./node_modules/@angular/compiler/fesm2022/compiler.mjs
Module build failed (from ../../code/frameworks/angular/node_modules/@angular-devkit/build-angular/src/tools/babel/webpack-loader.js):
BrowserslistError: [BABEL] /Users/yannbraga/open-source/storybook/sandbox/angular-cli-default-ts/node_modules/@angular/compiler/fesm2022/compiler.mjs: Unknown version 116 of edge (While processing: "base$0$0")
    at Function.select (/Users/yannbraga/open-source/storybook/code/node_modules/browserslist/index.js:1105:17)
    at /Users/yannbraga/open-source/storybook/code/node_modules/browserslist/index.js:319:29
    at Array.reduce (<anonymous>)

3:

Add only entry points to the 'files' or 'include' properties in your tsconfig.
./src/stories/frameworks/angular/core/moduleMetadata/angular-src/token.component.ts is part of the TypeScript compilation but it's unused.
Add only entry points to the 'files' or 'include' properties in your tsconfig.

4:

DefinePlugin
Conflicting values for 'process.env.NODE_ENV'

can you take a look into that?

@Marklb
Copy link
Member Author

Marklb commented Sep 4, 2023

@yannbf
1: I thought this only happened on the angular-cli/15-ts sandbox, but I will check that again.

2: This why I regenerated the lock file, so I have been regenerating the lock file when I merge next or at least reinstalling dependencies. I merged next and did not have this problem.

3: I haven't seen this before, but I will check. More strict typescript rules may have introduced this. I have seen it in an Angular project, but based on the sandbox's tsconfig I don't see why it would happen. This is new, since I started this PR.

4: This a warning that I have seen off and on for years, but I don't know what is causing it. Stricter typescript rules may have made it an error.

Even with the errors from 3 and 4, the sandbox seems to still work.

@ndelangen ndelangen changed the title Fix Angular sandboxes Sandbox: Fix angular Sep 5, 2023
@ndelangen ndelangen changed the title Sandbox: Fix angular Build: Fix angular sandbox Sep 5, 2023
@valentinpalkovic
Copy link
Contributor

Hey @Marklb,

Thank you for your contribution!

This PR is superseded by #24367.

I have updated the yarn.lock file once again to fix an issue related to the Angular 15 sandbox on our CI. Another issue popped up and it seems, that your fix isn't needed anymore.

I am closing this PR for now. Let me know if we should reopen it.

@Marklb
Copy link
Member Author

Marklb commented Nov 30, 2023

It would be nice to not have to alter the default config, but reverting #24373 and applying the three "compilerOptions" from this PR are the only way I have been able to use the sandbox locally. --no-link seems to expect prod builds of all the packages and I didn't ever figure out how to do that in a way that would actually work, because I just kept running into different problems.

If we want to reopen this PR, I can update this branch.

@valentinpalkovic valentinpalkovic changed the title Build: Fix angular sandbox Build: Fix Angular sandbox Dec 1, 2023
Copy link
Contributor

@valentinpalkovic valentinpalkovic left a comment

Choose a reason for hiding this comment

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

LGTM!

@valentinpalkovic valentinpalkovic merged commit e206035 into storybookjs:next Dec 4, 2023
49 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
angular bug build Internal-facing build tooling & test updates ci:normal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants