fix: merge resolvers #2049
fix: merge resolvers #2049
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2049 +/- ##
==========================================
- Coverage 68.82% 68.31% -0.52%
==========================================
Files 82 77 -5
Lines 2435 2408 -27
Branches 493 496 +3
==========================================
- Hits 1676 1645 -31
- Misses 759 763 +4
Continue to review full report at Codecov.
|
| return finalOptions; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Let's move it to WebpackCLI classs as method resolveArgs
There was a problem hiding this comment.
yes we can move but then it will be a big file, still continue?
There was a problem hiding this comment.
I don't think it will be very big, keeping logic in one places sometimes is better than separate it, this will help us in future improvements and refactor to see everything
|
And let's fix tests |
|
Great, I will merge after CI green, next - let's refactor and resolving config as method |
|
Yep |
|
@anshumanv tests still broken |
|
Yes wip 😄 |
| @@ -48,7 +48,8 @@ class WebpackCLI { | |||
|
|
|||
| async resolveArgs(args, configOptions = {}) { | |||
| // Since color flag has a default value, when there are no other args then exit | |||
| if (Object.keys(args).length === 1) return {}; | |||
| // eslint-disable-next-line no-prototype-builtins | |||
| if (Object.keys(args).length === 1 && args.hasOwnProperty('color')) return {}; | |||
There was a problem hiding this comment.
Why we have color property here?
There was a problem hiding this comment.
because color is default in args, we only need to skip it in that case
There was a problem hiding this comment.
in tests we're passing properties which are not color
There was a problem hiding this comment.
hm, why we have color in args?
There was a problem hiding this comment.
I think default is assigned there, will take a look and remove next
There was a problem hiding this comment.
Yep, let's do it in the new PR
There was a problem hiding this comment.
WIP already on color #2042 (comment)
What kind of change does this PR introduce?
refactor
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
NA
Summary
This merges multiple arg resolvers so we don't run them all separately even when they're simple. Not merged with config resolver as it's quite complex.
Does this PR introduce a breaking change?
No
Other information