-
-
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
Angular: Add styles and stylePreprocessorOptions to angular builder #16675
Conversation
This `onOf` is no more necessary
…icated styles config Allow the angular project to set styles config without using `browserTarget` in order to rely on another builder's config. Very useful in the case of a library where you don't have an application but you want to configure styles in storybook like an app
Nx Cloud ReportCI ran the following commands for commit 3beb736. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch
Sent with 💌 from NxCloud. |
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.
@ThibaudAV there are some failing unit tests -- can you please take a look?
…icated styles config Allow the angular project to set styles config without using `browserTarget` in order to rely on another builder's config. Very useful in the case of a library where you don't have an application but you want to configure styles in storybook like an app
Oups, thanks |
This didn't work in my library, but I will try to debug and see if I am just not using it correctly or if there is an error. 1. What I triedI added the following "storybook" and "build-storybook" targets to my library config, with the "styles" array from the "application" config that I currently use. It didn't error, but also didn't include the styles. "my-lib": {
"projectType": "library",
"root": "projects/my-lib",
"sourceRoot": "projects/my-lib",
"prefix": "lib",
"architect": {
"build": {
// Library build config
...
},
"storybook": {
"builder": "@storybook/angular:start-storybook",
"options": {
"browserTarget": "my-lib:build",
"port": 6006,
"compodoc": false,
"styles": [
"projects/my-lib/styles/theme.scss",
"node_modules/@marklb/ngx-datatable/assets/icons.css"
]
}
},
"build-storybook": {
"builder": "@storybook/angular:build-storybook",
"options": {
"browserTarget": "my-lib:build",
"compodoc": false,
"styles": [
"projects/my-lib/styles/theme.scss",
"node_modules/@marklb/ngx-datatable/assets/icons.css"
]
}
}
}
} 2. Questions
Example: "my-lib": {
"projectType": "library",
"root": "projects/my-lib",
"sourceRoot": "projects/my-lib",
"prefix": "lib",
"architect": {
"build": {
// Library build config
...
},
"storybook": {
"builder": "@storybook/angular:start-storybook",
"options": {
"browserTarget": "my-lib:build-storybook",
"port": 6006
}
},
"build-storybook": {
"builder": "@storybook/angular:build-storybook",
"options": {
"browserTarget": "my-lib:build",
"compodoc": false,
"styles": [
"projects/my-lib/styles/theme.scss",
"node_modules/@marklb/ngx-datatable/assets/icons.css"
]
}
}
}
} That would make the config reading more complicated, because "build-storybook" would need to read "build" then "storybook" would need to read "build-storybook" with the config it read from "build", but I am wondering if that would be worth it. It would save around ~110 lines in my |
On my lib it works you can test with the PR: gravitee-io/gravitee-ui-particles#13
The
related to what I said above ?
I see what you mean but I find that it does not help to understand. That would be really twisted 😁 I will say more that but if the configuration of storybook becomes as complex as that of browser builder. Then we can make a |
📝 This feature is for future versions of angular. And allows to complete a lack for the angular 13 support of storybook
|
That makes sense why my library wasn't working with ng12.0.5. I was thinking the description meant that it added additional logic for 12.2.x and >= 13, but assumed the existing versions supported by Storybook would still work. I will take another look at my library and see if I can get it working tonight. From what I tried in a new project, the "@storybook/angular:start-storybook" builder seems to be working. I was unable to try the "@storybook/angular:build-storybook" builder, because it was throwing an error that I didn't have time to debug at the moment. The error also happens when running the The only thing I saw that seems like a problem is the tsConfig file used. I would expect the The following is notes I took while building and configuring an ng12.2.0 project with just a root application then adding a library. The repo containing the following branches is: https://github.com/Marklb/sb-repro-16675-ng12.2.0 Branch:
|
Thanks for the feedback @Marklb. So if I understand correctly it's ok for the subject of this PR. I propose to fix it in this PR too with this one. With this resolve :
|
4397e21
to
fcb5a01
Compare
fcb5a01
to
bb8143b
Compare
I just tried again with newly generated Angular12.2 and Angular13 projects, with the following results. The following results are not for a project containing a library. I will try a library project next, but I expect similar results. SetupAngular 12.2 setup$ npx -p @angular/[email protected] ng new example --style=scss --routing=false
$ npx [email protected] init // This installed "^6.4.0-rc.7"
// Then I manually set all storybook packages to "6.4.0-rc.5" in `package.json`.
$ npm i
// Then I started a local verdaccio server, with packages built from this PR's branch.
// Then I manually set all storybook packages to "6.4.0-rc.7" in `package.json`.
$ npm i Angular 13 setup$ npx -p @angular/[email protected] ng new example --style=scss --routing=false
$ npx [email protected] init // This installed "^6.4.0-rc.7"
// Then I manually set all storybook packages to "6.4.0-rc.5" in `package.json`.
$ npm i
// Then I started a local verdaccio server, with packages built from this PR's branch.
// Then I manually set all storybook packages to "6.4.0-rc.7" in `package.json`.
$ npm i TestingBoth worked fine, with "@storybook/angular:start-storybook" builder
"@storybook/angular:build-storybook" builder
|
I just added a library to the projects I previously tested and got similar results. So, at the moment, I think fixing the "styles" and "stylePreprocessorOptions" not inheriting from the "browserTarget" config and the "allowSyntheticDefaultImports" compilerOption being required when using the builders are the only two things the probably need to be fixed for this PR. |
@Marklb Thanks again actually builder with json validation do things I hadn't planned in this way 😱 It should be better |
@ThibaudAV Do you mean that not getting styles from the "browserTarget" is or isn't intended? The schema description implies that it does get styles from there. If not getting the styles from "browserTarget" is intended then it seems that it would add additional required configuration to manage that most projects have no reason to change from what "build" uses. I do get that "test" builder separately defines them, so I am still not fully convinced which way it should work, yet. To me it seems confusing how less than ng12.2 doesn't support styles the same way. Is that intended to be added by another PR? Do you intend "assets" and "scripts" will be changed to work the same way? I know that my projects can't use the builder unless "assets" work, which should for mine at the moment, because I use a builder that wraps the ng-packagr builder and adds "assets" to the configuration for library projects. |
I didn't expect the angular builder to add an empty value if styles and stylePreprocessorOptions are not filled in, which caused the problem you mentioned before.
Same for stylePreprocessorOptions
Not planned by myself. (or only for ng14 ^^) not the subject here. but go ahead if you want
not the subject here. and for |
@ThibaudAV @Marklb thanks so much for hustling on this. in the interest of getting it available for the community to test, i'm going to merge and release this now. if there's still more work to be done here, let's do it in a follow-up PR! 🙏 |
Issue: #16763 #14612 #15246 #15855
Not solve but provide a solution / or related for
What I did
Add
styles
andstylePreprocessorOptions
options storybook angular builderAllow the angular 12.2.x and >=13 project to set styles config without using
browserTarget
in order to rely on another builder's config.Very useful in the case of a library where you don't have an application but you want to configure styles in storybook like an app
How to test
If your answer is yes to any of these, please make sure to include it in your PR.