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

chore: Add tests #173

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

chore: Add tests #173

wants to merge 2 commits into from

Conversation

bebraw
Copy link
Member

@bebraw bebraw commented Jan 12, 2021

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.

@codecov-io
Copy link

codecov-io commented Jan 17, 2021

Codecov Report

Merging #173 (638926e) into develop (28cfdbd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28cfdbd...638926e. Read the comment docs.

],
},
};
// exclude is discarded because it's not in the base configuration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few other options:

  1. Go with the default behavior (which is Append), so that the resulting configuration would contain the exclude array from the second object. In this case though I'd expect that I would be able to match between exclude fields 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.
  2. Add explicit Omit behavioural option. Which would mean "merge as usual and Omit any appearance of this property when met".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use a default behavior we have to add an option to avoid that. Like what I mentioned - add match for exclude.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, as opposed to the previous one, exclude has explicit merge rule, and everything else matches, so it seems legit to me.

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.

4 participants