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

fix(custom-webpack): specify possible types of customWebpackConfig values #971

Merged
merged 4 commits into from
May 2, 2021

Conversation

juristr
Copy link
Contributor

@juristr juristr commented Apr 19, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

At the moment if someone uses this with the latest versions of Nx and does not specify a webpack.config.js the build fails because it requires one.

The reason is how the defaults are being interpreted by the Angular CLI vs Nx schema parser. Right now the custom-webpack browser schema defines customWebpackConfig to be of type object but assigns a boolean value as the default. The Angular CLI is less strict in that it accepts that, while Nx is more strict (I'm working on the Nx side to avoid such edge cases as much as possible).

What is the new behavior?

This PR adjusts the schema to properly reflect the possible types, basically a object or boolean. This is achieved with the oneOf property in the schema file.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@juristr juristr force-pushed the fix-custom-webpack-schema branch from 873ceb0 to 557d25b Compare April 19, 2021 16:18
@just-jeb
Copy link
Owner

I think the default value false is a leftover from the very early days of the builder.
The right type would be then object or undefined with no default value (this should fix the problem with Nx as well).

@juristr
Copy link
Contributor Author

juristr commented Apr 21, 2021

@just-jeb passing undefined/null or no default value wouldn't work as the Angular CLI (and Nx) initialize those values with a default object.

image

Thus the check in the CustomWebpackBuilder fails and would require the user to specify a custom webpack configuration.

I don't think using false as the default value is wrong at all, it's just the schema that needs to be adjusted to account for that. Very similar to what the Angular devkit builder does for the optimization property. Let me know what you think

@juristr
Copy link
Contributor Author

juristr commented Apr 27, 2021

@just-jeb Sorry for pinging 😅. Any opinion on the above?

@just-jeb
Copy link
Owner

just-jeb commented May 2, 2021

Sounds good to me

@just-jeb
Copy link
Owner

just-jeb commented May 2, 2021

@all-contributors please add @juristr for code

@allcontributors
Copy link
Contributor

@just-jeb

I've put up a pull request to add @juristr! 🎉

@just-jeb just-jeb merged commit 01f5427 into just-jeb:master May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants