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

mergeWithRules and different rules for different properties #169

Open
just-jeb opened this issue Dec 16, 2020 · 11 comments
Open

mergeWithRules and different rules for different properties #169

just-jeb opened this issue Dec 16, 2020 · 11 comments

Comments

@just-jeb
Copy link
Contributor

just-jeb commented Dec 16, 2020

In continuation to #167.
What would be the expected output to the following two objects merge?

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"
                  ]
                }
              }
            ]
          }
        ]
      }
    };

with the following rules?

    const mergeRules = {
      module: {
        rules: {
          test: CustomizeRule.Match,
          use: {
            loader: CustomizeRule.Match,
            options: CustomizeRule.Merge,
          },
        },
      },
    }

And what if I change the rules to the following?

    const mergeRules = {
      module: {
        rules: {
          test: CustomizeRule.Match,
          excluded: CustomizeRule.Append,
          use: {
            loader: CustomizeRule.Match,
            options: CustomizeRule.Merge,
          },
        },
      },
    }

Is this even legit?

@bebraw
Copy link
Member

bebraw commented Dec 16, 2020

Since both test and loader won't match for the first one, I would expect it to go with the default behavior and merge as separate rules within the same array.

The second one is harder as Append is in-between but I might go with the same here and look for the total match. The in-between case didn't come up before so I'm not sure if that should even be valid.

@just-jeb
Copy link
Contributor Author

Since both test and loader won't match for the first one, I would expect it to go with the default behavior and merge as separate rules within the same array.

I'd expect the same, but that's not what's happening. exclude does not appear in the resulting object at all...

@just-jeb
Copy link
Contributor Author

just-jeb commented Dec 16, 2020

Actually, not quite correct... The test matches but the loader doesn't. But exclude is the test's sibling, not a child, so I'm not sure what's the expected behavior.

I mean, what you said would be a perfect solution for me, since this is what I would expect from the webpack configs merge.
But does it still stand true if we look at it as a generic merge?

@bebraw
Copy link
Member

bebraw commented Dec 16, 2020 via email

@just-jeb
Copy link
Contributor Author

So what would it be? Two separate entries in the rules array or something else?

@bebraw
Copy link
Member

bebraw commented Dec 16, 2020 via email

@just-jeb
Copy link
Contributor Author

Looking forward to seeing it! Thanks!

@just-jeb
Copy link
Contributor Author

Another question. What if we had a match in both test and loader but had additional fields that are different (like exclude and include).
How should it behave then?

const conf1 = {
      module: {
        rules:[
          {
            "test": "/\\.scss$|\\.sass$/",
            "include": ["/node_modules/"]
            "use": [
              {
                "loader": "sass-loader",
                "options": {
                  "sourceMap": true
                }
              }
            ]
          }
        ]
      }
    };
    const conf2 = {
      module: {
        rules:[
          {
            "test": "/\\.scss$|\\.sass$/",
            "exclude": ["/node_modules/"],
            "use": [
              {
                "loader": "sass-loader",
                "options": {
                   anotherOption: "blah"
                }
              }
            ]
          }
        ]
      }
    };
    const mergeRules = {
      module: {
        rules: {
          test: CustomizeRule.Match,
          use: {
            loader: CustomizeRule.Match,
            options: CustomizeRule.Merge,
          },
        },
      },
    }

@bebraw
Copy link
Member

bebraw commented Dec 17, 2020

@just-jeb That's a good one I need to test. I think it should merge those two matches and fall back to the default merge behavior for anything outside of the explicit definition. That means it would pick up both include and exclude. Of course semantically it's a contradiction and I don't know what webpack will do in this case (which will win?).

It's a good example of the flexibility of webpack loader configuration. In my personal use, I always go with include as it's more explicit and predictable than exclude.

@just-jeb
Copy link
Contributor Author

Doesn't it contradict a bit the previous example?
On one hand, we say that if there is just a partial match, (like in the first post) then we go with the highest common ancestor (the rules in this case).
On the other hand we say that if there is a full match then we merge the matching object while merging everything along the way with the default behavior. In this case we should at least provide the option to configure the way the properties are merged along the way, don't you think?

I think I'm missing a solid definition of how the mergeWithRules should work and what is the expected behavior. I mean the docs say:

To support advanced merging needs (i.e. merging within loaders), mergeWithRules includes additional syntax that allows you to match fields and apply strategies to match.

But it doesn't really explain how it should work. If it at least explained how it works as for today (based on the implemented algorithm) it would be really helpful I think.

@bebraw
Copy link
Member

bebraw commented Oct 16, 2023

#213 just landed. It's possible it helps with this particular use case or at least can help to find a solution for it.

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

No branches or pull requests

3 participants
@bebraw @just-jeb and others