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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b488d2
chore: Add tests
bebraw Jan 12, 2021
638926e
chore: Drop a todo
bebraw Jan 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
209 changes: 179 additions & 30 deletions test/merge-with-rules.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1118,16 +1118,14 @@ describe("Merge with rules", function () {
).toEqual(result);
});

it('should not merge strings with objects', () => {
it("should not merge strings with objects", () => {
const conf1 = {
module: {
rules: [
{
"test": "some-test",
"use": [
"hello-loader"
]
}
test: "some-test",
use: ["hello-loader"],
},
],
},
};
Expand All @@ -1136,40 +1134,114 @@ describe("Merge with rules", function () {
module: {
rules: [
{
"test": "another-test",
"use": [
test: "another-test",
use: [
{
"loader": "another-loader",
"options": {
"someoption": "hey"
}
}
]
}
]
loader: "another-loader",
options: {
someoption: "hey",
},
},
],
},
],
},
};

const expected = {
module: {
rules: [
{
"test": "some-test",
"use": [
"hello-loader"
]
test: "some-test",
use: ["hello-loader"],
},
{
test: "another-test",
use: [
{
loader: "another-loader",
options: {
someoption: "hey",
},
},
],
},
],
},
};

expect(
mergeWithRules({
module: {
rules: {
test: CustomizeRule.Match,
use: {
loader: CustomizeRule.Match,
options: CustomizeRule.Merge,
},
},
},
})(conf1, conf2)
).toEqual(expected);
});

it("should merge with same with different options - 1 #169", () => {
const conf1 = {
module: {
rules: [
{
test: "/\\.scss$|\\.sass$/",
use: [
{
loader: "sass-loader",
options: {
sourceMap: true,
},
},
],
},
],
},
};
const conf2 = {
module: {
rules: [
{
test: "/\\.scss$|\\.sass$/",
exclude: ["/node_modules/"],
use: [
{
loader: "sass-resources-loader",
options: {
resources: ["src/styles/includes.scss"],
},
},
],
},
],
},
};
// 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.

const expected = {
module: {
rules: [
{
"test": "another-test",
"use": [
test: "/\\.scss$|\\.sass$/",
use: [
{
loader: "sass-loader",
options: {
sourceMap: true,
},
},
{
"loader": "another-loader",
"options": {
"someoption": "hey"
}
}
]
}
loader: "sass-resources-loader",
options: {
resources: ["src/styles/includes.scss"],
},
},
],
},
],
},
};
Expand All @@ -1187,6 +1259,83 @@ describe("Merge with rules", function () {
},
})(conf1, conf2)
).toEqual(expected);
})
});

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.

const conf1 = {
module: {
rules: [
{
test: "/\\.scss$|\\.sass$/",
exclude: [],
use: [
{
loader: "sass-loader",
options: {
sourceMap: true,
},
},
],
},
],
},
};
const conf2 = {
module: {
rules: [
{
test: "/\\.scss$|\\.sass$/",
exclude: ["/node_modules/"],
use: [
{
loader: "sass-resources-loader",
options: {
resources: ["src/styles/includes.scss"],
},
},
],
},
],
},
};
const expected = {
module: {
rules: [
{
test: "/\\.scss$|\\.sass$/",
exclude: ["/node_modules/"],
use: [
{
loader: "sass-loader",
options: {
sourceMap: true,
},
},
{
loader: "sass-resources-loader",
options: {
resources: ["src/styles/includes.scss"],
},
},
],
},
],
},
};

expect(
mergeWithRules({
module: {
rules: {
test: CustomizeRule.Match,
exclude: CustomizeRule.Append,
use: {
loader: CustomizeRule.Match,
options: CustomizeRule.Merge,
},
},
},
})(conf1, conf2)
).toEqual(expected);
});
});