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 misjudges the rules.test equivalent #212

Closed
RexSkz opened this issue Oct 13, 2023 · 5 comments · Fixed by #213
Closed

mergeWithRules misjudges the rules.test equivalent #212

RexSkz opened this issue Oct 13, 2023 · 5 comments · Fixed by #213

Comments

@RexSkz
Copy link
Contributor

RexSkz commented Oct 13, 2023

The issue happens on the latest version of webpack-merge.

Brief Description

Assume we have two tests using object:

// match all .less files in Ant Design
{ and: [/\.less$/, /antd/] }

// match all .less files in Material UI
{ and: [/\.less$/, /@mui/] }

They should be converted using less-loader but with different options, the value of use[0].options.lessOptions should be:

// for Ant Design
{ modifyVars: { a: 1 } }

// for Material UI
{ modifyVars: { b: 2 } }

The merge result is not as expected, because webpack-merge uses the toString result to compare the test value:

const matches = rulesToMatch.every(
  (rule) => ao[rule]?.toString() === o[rule]?.toString()
);

The two results are both "[object Object]", causing the latter rule to be missing and its lessOptions to override the former one.

Full Example

Run this script:

const { mergeWithRules } = require('webpack-merge');

const a = {
  module: {
    rules: [
      {
        test: { and: [/\.less$/, /antd/] },
        use: [
          {
            loader: 'less-loader',
            options: {
              lessOptions: {
                modifyVars: { a: 1 },
              },
            },
          },
        ],
      },
    ],
  },
};

const b = {
  module: {
    rules: [
      {
        test: { and: [/\.less$/, /@mui/] },
        use: [
          {
            loader: 'less-loader',
            options: {
              lessOptions: {
                modifyVars: { b: 2 },
              },
            },
          },
        ],
      },
    ],
  },
};

const result = mergeWithRules({
  module: {
    rules: {
      test: 'match',
      use: {
        loader: 'match',
        options: 'replace',
      },
    },
  },
})(a, b);

console.log(JSON.stringify(result, (_, value) => (
  value instanceof RegExp ? value.toString() : value
), 2));

And we will get:

{
  "module": {
    "rules": [
      {
        "test": {
          "and": [
            "/\\.less$/",
            "/antd/"
          ]
        },
        "use": [
          {
            "loader": "less-loader",
            "options": {
              "lessOptions": {
                "modifyVars": {
                  "b": 2
                }
              }
            }
          }
        ]
      }
    ]
  }
}

Suggestion

Using the deep equal (e.g. _.isEqual in lodash) to compare the test value.

@bebraw
Copy link
Member

bebraw commented Oct 13, 2023

Can you send a PR? This package isn't maintained due to lack of funding but I can merge fixes.

@RexSkz
Copy link
Contributor Author

RexSkz commented Oct 13, 2023

Sure, PR is on the way~

@RexSkz
Copy link
Contributor Author

RexSkz commented Oct 16, 2023

Hello @bebraw, PR #213 fixes this issue and does not affect the scenarios that currently work; you can trigger the pipeline and have a look at my code :)

@bebraw
Copy link
Member

bebraw commented Oct 16, 2023

Yup, thanks. I will have a good look today.

@bebraw
Copy link
Member

bebraw commented Oct 16, 2023

Included in 5.10.0. Thanks a lot for your contribution.

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 a pull request may close this issue.

2 participants