-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Downgrade non-default builders error to warning #14176
Comments
So, if someone uses non-default CLI builders, you expect the schematic to still run but just with a warning? I'm wondering how that would work if someone runs |
Yes. That's a good question. Skipping it seems appropriate. I wonder if there's a way to see if a builder accepts some option by reading the schema. But skipping with a link to the docs in the message is a good approach. Another issue would be reading from the config as well. I guess a good example of this would be someone with a custom webpack build using the webpack builder. If you don't feel comfortable with downgrading to a warning for build because it's so integral to the schematic, I understand. I imagine other targets like test are not as integral. |
I see, reading the Maybe we can work on some public API (maybe there already is one, not too sure yet), and then we can consider using that API in order to check if there is a |
Downgrading to a warning feels a bit odd because the styles are a major part of setting up Angular Material, and leaving that setup step in some cases feels like missing the actual |
I am fine with that approach. Recap/ Requirements:
Let me know if this makes sense to you. Thank you! |
* No longer throws an exception if someone runs `ng-add` but uses a custom builder for the `test` target. Since theme files are not necessarily mandatory for running tests, we should just show a warning instead of exiting the whole `ng-add` process. Fixes angular#14176
* No longer throws an exception if someone runs `ng-add` but uses a custom builder for the `test` target. Since theme files are not necessarily mandatory for running tests, we should just show a warning instead of exiting the whole `ng-add` process. Fixes angular#14176
@FrozenPandaz Sounds reasonable. Here is the PR: #14203 |
* No longer throws an exception if someone runs `ng-add` but uses a custom builder for the `test` target. Since theme files are not necessarily mandatory for running tests, we should just show a warning instead of exiting the whole `ng-add` process. Fixes #14176
* No longer throws an exception if someone runs `ng-add` but uses a custom builder for the `test` target. Since theme files are not necessarily mandatory for running tests, we should just show a warning instead of exiting the whole `ng-add` process. Fixes #14176
…ar#14203) * No longer throws an exception if someone runs `ng-add` but uses a custom builder for the `test` target. Since theme files are not necessarily mandatory for running tests, we should just show a warning instead of exiting the whole `ng-add` process. Fixes angular#14176
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Bug, feature request, or proposal:
Feature request for schematics
What is the expected behavior?
Not having the default builders and using the schematics should throw a warning.
Ideally, there would also be an option to silence it.
What is the current behavior?
Having non-default builders throws an error
What are the steps to reproduce?
Providing a StackBlitz reproduction is the best way to share your issue.
StackBlitz starter: https://goo.gl/wwnhMV
npx @nrwl/schematics my-proj --no-interactive cd my-proj ng g app app1 --unit-test-runner jest --no-interactive ng add @angular/material
What is the use-case or motivation for changing an existing behavior?
Users should be able to use their own builders at their own risk.
Which versions of Angular, Material, OS, TypeScript, browsers are affected?
Whichever versions material enforce the builders (6+?)
Is there anything else we should know?
Original Issue:
nrwl/nx#868
The text was updated successfully, but these errors were encountered: