chore: Add tests #173
chore: Add tests #173
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #173 +/- ##
========================================
Coverage 99.14% 99.14%
========================================
Files 6 6
Lines 233 233
Branches 64 64
========================================
Hits 231 231
Misses 2 2 Continue to review full report at Codecov.
|
| ], | ||
| }, | ||
| }; | ||
| // exclude is discarded because it's not in the base configuration |
There was a problem hiding this comment.
I don't think that it's a valid behavior. At least not something I'd expect from a merge function.
It's OK when merge overrides stuff, but not when it omits stuff.
I mean when you merge two objects you expect the result to contain data from both, not a subset of data.
That said, this merge is unusual, because it's a match merge, so it merges objects only if there is a full match.
Problem is, you cannot merge partially, because you merge a tree. And this is something we need to focus on I believe.
The match merge can work only if there is a complete match on the defined subtree.
Thus, I'd say that a logical behavior in this case is that when you have a match on some property but not on its sibling you cannot merge any children (unless you want to merge this sibling with explicit rule).
I have to say that it's opinionated, because I come from the webpack perspective. Maybe we should use some other example (not webpackee) and see how it works.
WDYT?
There was a problem hiding this comment.
A few other options:
- Go with the default behavior (which is
Append), so that the resulting configuration would contain theexcludearray from the second object. In this case though I'd expect that I would be able to match betweenexcludefields as well, in order to avoid this behavior (to say "only if there is a match in BOTH 'test' and 'exlclude' then merge"). Currently I think it's not possible. - Add explicit
Omitbehavioural option. Which would mean "merge as usual and Omit any appearance of this property when met".
There was a problem hiding this comment.
Yeah, I agree it probably should merge by default in this case even if the parent is missing fields as otherwise the outcome is a bit weird. I'll alter the PR to reflect that.
We can still add Omit later on if the use case comes up.
There was a problem hiding this comment.
If we use a default behavior we have to add an option to avoid that. Like what I mentioned - add match for exclude.
There was a problem hiding this comment.
Because specifically in webpack this behavior is undesired - you don't want to add exclude to a loader that doesn't have exclude. In webpack you'd want these loaders to be two separate entries, one with exlcude and the other without.
There was a problem hiding this comment.
Yeah, I see. Now that I think of it, we have something that should capture this case. It's the purpose of the Replace strategy. I wonder if it replaces if the same field is missing from the parent, though. Maybe not.
Can you fork my PR and adjust the tests to the way you think it should work? The tests don't have to pass. I think that would be a useful step to make sure we are aligned on the thinking here.
| }) | ||
| }); | ||
|
|
||
| it("should merge with same with different options - 2 #169", () => { |
There was a problem hiding this comment.
In this case, as opposed to the previous one, exclude has explicit merge rule, and everything else matches, so it seems legit to me.
Related to #169.
There are a couple of points to discuss.
@just-jeb When you have time, could you have a quick look? We should specify the behavior for these two cases.