-
Notifications
You must be signed in to change notification settings - Fork 94
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
Change match() to pass all webpack rule options to context. #250
Conversation
7fe8df7
to
d345ba5
Compare
packages/core/CHANGELOG.md
Outdated
@@ -1,5 +1,9 @@ | |||
# @webpack-blocks/core - Changelog | |||
|
|||
## master | |||
|
|||
- Change `match()` to passes all [webpack rule options](https://webpack.js.org/configuration/module/) to context. ([#250](https://github.com/andywer/webpack-blocks/pull/250)) |
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.
passes -> pass
I was wrong, the |
packages/core/lib/match.js
Outdated
@@ -23,20 +21,13 @@ function match (test, options, configSetters) { | |||
|
|||
assertConfigSetters(configSetters) | |||
|
|||
const match = { test: createFileTypeMatcher(test) } | |||
options.test = createFileTypeMatcher(test) |
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.
We should print a warning if test
was set in options.
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.
Yeah, if options.test
was set before this line 👍
packages/core/lib/match.js
Outdated
@@ -10,8 +10,6 @@ module.exports = match | |||
* | |||
* @param {string|RegExp|Function|Array} test A glob like `*.css` or `{*.js, *.jsx}` or something else to use as `loader.test`. | |||
* @param {object} [options] Optional advanced matching options. | |||
* @param {string|Function|RegExp|Array|object} [options.include] |
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.
If we remove those hints, then we definitely need to improve the documentation of the overall options
parameter, since otherwise it's not clear anymore what kind of options can be passed here 😉
packages/core/CHANGELOG.md
Outdated
@@ -1,5 +1,9 @@ | |||
# @webpack-blocks/core - Changelog | |||
|
|||
## master |
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.
master -> Next version (like we used before)
Updated |
packages/core/lib/match.js
Outdated
@@ -21,6 +21,10 @@ function match (test, options, configSetters) { | |||
|
|||
assertConfigSetters(configSetters) | |||
|
|||
if (options.test) { | |||
console.warn(`match(): Setting 'test' in options is not supported and will be overriden with a 'test' argument.`) |
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.
Maybe we should even throw! What do you think?
Seems useful to defuse the bomb instead of putting a warning sign on it.
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.
Thanks for this change, @vlad-zhukov!
Just left a comment to maybe turn the warning into a throw
. But would be fine with merging either way.
6b8cd67
to
27591f7
Compare
27591f7
to
be05cf4
Compare
#253 unintentionally mostly covered this PR. |
Currently it's not possible to pass webpack rule options other than
exclude
andinclude
to blocks. Also there is no way to remove default options from blocks (for example theexclude
in thebabel
block).This PR fixes both problems.