Support Config.node false value #252
Conversation
SuperOleg39
force-pushed
the
config-node-bool
branch
from
8b22501
to
a45b512
Compare
SuperOleg39
force-pushed
the
config-node-bool
branch
from
d63e1e5
to
69a58a2
Compare
There was a problem hiding this comment.
One problem I have with this approach is that doing config.set('node', {}) will force the node property to never show up in the final config, which might end up being extremely confusing to a user who might not expect config.delete('node') to be the way to bring back the standard behavior. Similarly, using config.set('node', []) will cause even more weirdness to happen.
I see three potential solutions to this problem:
- Ignore this and call it a feature,
- Ignore any value other than
false, - Override the
config.setfunction and raise an error.
I'm not sure what the standard is in this library, so I'm just mentioning it and leaving any decisions up to the maintainers. If the first option is chosen, I'd recommend at least changing the documentation accordingly.
| You can set `node` option to object, or a `false` value. | ||
| `false` value always rewrite `config.node` object. |
There was a problem hiding this comment.
| You can set `node` option to object, or a `false` value. | |
| `false` value always rewrite `config.node` object. | |
| You can set the `node` option to an object, or a `false` value. | |
| A `false` value always overrides the `config.node` chained map. |
| `false` value always rewrite `config.node` object. | ||
|
|
||
| ```js | ||
| config.set('node', false); |
There was a problem hiding this comment.
| config.set('node', false); | |
| config.set('node', false); | |
| // To use the value generated from `config.node` ChainedMap again | |
| config.delete('node'); |
| @@ -165,12 +166,21 @@ module.exports = class extends ChainedMap { | |||
| ); | |||
| } | |||
|
|
|||
| if (!omit.includes('node') && 'node' in obj) { | |||
| if (typeof obj.node === 'object') { | |||
There was a problem hiding this comment.
true for obj.node == []. Though the input is unexpected it might cause issues.
|
I'd also like to mention the possibility of creating callable classes even though I feel it's rather hacky. Something like this could also be used for #222 (though that one could be resolved without weird hacks by introducing a breaking change and bumping the major). |
|
@SuperOleg39 thank you for the PR and @opl- thank you for the really helpful review/thoughts. I don't use webpack day to day any more, so am reliant on suggestions as to what direction it should take longer term. I agree with @opl- 's thoughts about potentially confusing UX. What do you both think as the best option out of those that are proposed above? |
|
Closing since this project is no longer maintained: |
Resolves #209.