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

Downgrade non-default builders error to warning #14176

Closed
FrozenPandaz opened this issue Nov 16, 2018 · 7 comments · Fixed by #14203
Closed

Downgrade non-default builders error to warning #14176

FrozenPandaz opened this issue Nov 16, 2018 · 7 comments · Fixed by #14203
Assignees

Comments

@FrozenPandaz
Copy link

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

@FrozenPandaz FrozenPandaz changed the title Downgrade non-default builders to warnings Downgrade non-default builders error to warning Nov 16, 2018
@devversion devversion self-assigned this Nov 16, 2018
@devversion
Copy link
Member

devversion commented Nov 16, 2018

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 ng add @angular/material and we want to add a theme style to the build target. How would that work? just skipping that step?

@FrozenPandaz
Copy link
Author

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.

@devversion
Copy link
Member

I see, reading the config for a builder sounds very reasonable, but I'm afraid that this is out-of-scope for our Material schematics.

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 styles option available.

@devversion
Copy link
Member

devversion commented Nov 18, 2018

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 ng-add goal.

@FrozenPandaz
Copy link
Author

FrozenPandaz commented Nov 19, 2018

I am fine with that approach.

Recap/ Requirements:

  • Erroring on build for ng-add not matching the default builder because the schematic cannot deliver it's purpose without it.
    • For other schematics, if nothing is being altered in the angular.json, it may be nice if it is not an error.
  • Warning on test, lint, e2e not matching the default builder because the schematic can deliver it's purpose but it is not guaranteed to work and there is some risk.

For transparency, Nx does not alter the default build builder so checking of build doesn't affect Nx users at the moment.

Let me know if this makes sense to you.

Thank you!

devversion added a commit to devversion/material2 that referenced this issue Nov 19, 2018
* 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
devversion added a commit to devversion/material2 that referenced this issue Nov 19, 2018
* 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
@devversion
Copy link
Member

@FrozenPandaz Sounds reasonable. Here is the PR: #14203

jelbourn pushed a commit that referenced this issue Nov 28, 2018
* 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
jelbourn pushed a commit that referenced this issue Dec 3, 2018
* 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
josephperrott pushed a commit to josephperrott/components that referenced this issue Jan 14, 2019
…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
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants