Merging two module.rules just appends

[muuvmuuv]

This is the test I want to run but it seems that the latest version does not longer merge rules.

  it('Should merge loader options', () => {
    const output = merge(
      {
        module: {
          rules: [
            {
              test: /\.css$/,
              use: [{ loader: 'style-loader' }, { loader: 'sass-loader' }],
            },
          ],
        },
      },
      {
        module: {
          rules: [
            {
              test: /\.css$/,
              use: [
                {
                  loader: 'style-loader',
                  options: {
                    modules: true,
                  },
                },
              ],
            },
          ],
        },
      }
    );

    const expected = {
      module: {
        rules: [
          {
            test: /\.css$/,
            use: [
              {
                loader: 'style-loader',
                options: {
                  modules: true,
                },
              },
              { loader: 'sass-loader' },
            ],
          },
        ],
      },
    };

    expect(output).toEqual(expected);
  });

Output is:

  [
        { test: /\.css$/, use: [ [Object], [Object] ] },
        { test: /\.css$/, use: [ [Object] ] }
      ]

[bebraw]

Were you using smart merging before?

If so, I added features for customizing the merge to cover the cases it tried to handle before.

Do you have the whole configuration somewhere? I could try to propose a clean alternative for the use case.

[muuvmuuv]

Yep we have used smart merging before: https://github.com/just-jeb/angular-builders/blob/master/packages/custom-webpack/src/webpack-config-merger.ts#L12

Currently this is the update:

import { mergeWithCustomize, customizeObject, customizeArray } from 'webpack-merge';
import { Configuration } from 'webpack';
import { merge, remove } from 'lodash';
import { MergeStrategyRules } from './custom-webpack-builder-config';

export function mergeConfigs(
  webpackConfig1: Configuration,
  webpackConfig2: Configuration,
  mergeStrategies: MergeStrategyRules = {},
  replacePlugins = false
): Configuration {
// always remove duplicates from config 1 and replace with plugin from config 2
// if wanted merge config 1 plugin into 2
  if (webpackConfig1.plugins && webpackConfig2.plugins) {
    webpackConfig2.plugins = webpackConfig2.plugins.map(plugin => {
      const foundDuplicate = webpackConfig1.plugins.findIndex(
        p => p.constructor.name === plugin.constructor.name
      );
      if (foundDuplicate !== -1) {
        if (!replacePlugins) {
          plugin = merge(webpackConfig1.plugins[foundDuplicate], plugin);
        }
        remove(webpackConfig1.plugins, (_, i) => i === foundDuplicate);
      }
      return plugin;
    });
  }

  const mergedConfig = mergeWithCustomize({
    customizeArray: customizeArray(mergeStrategies),
    customizeObject: customizeObject(mergeStrategies),
  })(webpackConfig1, webpackConfig2);

  return mergedConfig;
}

[muuvmuuv]

I would propose a new merge strategy "merge" which would look like this in that specific case:

{
  "module.rules.*": "merge"
}

[bebraw]

@muuvmuuv Yeah, using a strategy would fit well. I'll go that way (might need a day or two due to other work).

[bebraw]

I gave this one initial go. It looks like adding a strategy won't work for this case as it's too complex. It contains a combination of uniqueness and merging which makes it tricky to map to the current semantics.

To work, it would have to perform a merge over module.rules while making sure test: /\.css$/ is unique and merges loader while treating that as a unique field.

Something like this:

const output = mergeWithCustomize({
  customizeArray: unique(
    "module.rules",
    // These are individual rules
    (a, b) => mergeWithCustomize({
      customizeArray: customizeArray({
        // Replace since we want the latter only for use that matches
        use: "replace"
      }),
    }),
    // Extract tests from rules so they can be merged
    rule => rule.test
  ),
})(configs);

I would have to add support for nested keys and extend the second parameter to unique but it feels like this approach might do it.

@muuvmuuv What do you think of the above?

[muuvmuuv]

Hey @bebraw, sorry for the late response. Since this is for a commercial project I haven't looked into it through the weekend. I will test this today but does this include a fallback for anything else than "module.rules"?

[bebraw]

@muuvmuuv Hi, the code above doesn't work yet. It's something I could develop on top of the current API without breaking the current design.

[muuvmuuv]

I am getting typing errors. Maybe it would be good to add webpack typings to the "unique" function, since webpack-merge is only webpack config.

Argument of type '() => (firstConfiguration: Configuration | Configuration[], ...configurations: Configuration[]) => Configuration' is not assignable to parameter of type 'string[]'.
  Type '() => (firstConfiguration: Configuration | Configuration[], ...configurations: Configuration[]) => Configuration' is missing the following properties from type 'string[]': pop, push, concat, join, and 25 more.ts(2345)

I also had to do this:

      (rule: RuleSetRule) => rule.test

[muuvmuuv]

Oh, sorry then I misunderstood that! So just take the above as hints ^^.

[bebraw]

Yeah, that's what I meant. I'll have to extend the signature and implementation. 👍

[muuvmuuv]

What do you think about an array or single input for customizeArray? That way it would be possible to pass the configs through different custom merge strategies.

  const output = mergeWithCustomize({
    customizeArray: [
      customizeArray(mergeStrategies), // merge by default rules
      unique( // add specific use case 
        'module.rules',
        () =>
          mergeWithCustomize({
            customizeArray: customizeArray({
              // replace since we want the latter only for use that matches
              use: 'replace',
            }),
          }),
        (rule: RuleSetRule) => rule.test
      )
      // another custom rule
      // now merge with a third config e.g.
      // etc. etc.
    ],
  })(webpackConfig1, webpackConfig2);

[bebraw]

@muuvmuuv Yeah, that's a good idea since then you could build more matchers. I have to check the code to understand the implications but I see the use case.

[bebraw]

I tried out the API proposal but it's not that simple as we have to check against test and use.

I found that you can do this with the current API:

merge({
  customizeArray(a, b, key) {
    if (key === "module.rules") {
      return a.map((v) => {
        let bv = b.find(
          ({ test }) => test.toString() === v.test.toString()
        );

        if (!bv) {
          return v;
        }

        return {
          ...v,
          use: v.use.map((uv) => {
            let bvv = bv.use.find(({ loader }) => loader === uv.loader);

            if (!bvv) {
              return uv;
            }

            return {
              ...uv,
              options: {
                ...(uv.options || {}),
                ...bvv.options,
              },
            };
          }),
        };
      });
    }

    return b;
  },
})(configs)

I think the next step is to write a helper to simplify the code above. It feels like there's something like mergeArrays(???) that's repeating twice. That will have to take over the map and some of the checks while accept a callback to use within the find. Maybe something like mergeArrays({ find: o => boolean, match: (a, b) => config, noMatch: (a, b) => config })(a, b). I'll try a simplification in the next pass.

The problem is a bit tricky as it doesn't fall into regular object merge and it has checks in it against specific fields.

[bebraw]

I think the best I can do will look something like this:

mergeArrays(
  'test',
  (a, b) => ({
    ...merge(a, b),
    use: mergeArrays('use', merge)
  })
)(a, b)

Since toString against strings is idempotent, we can simplify the API a notch. That feels better than the verbose example in the previous comment I think I should be able to reduce it to a form like this.

[bebraw]

I think this is the best I can do:

mergeWithCustomize({
  customizeArray: customizeArray({
    "module.rules[test].use[loader]": CustomizeRule.Replace
  })
})

I'll need to write some kind of parser to deal with the syntax and then map from that to the code but it's the perhaps the cleanest approach and it allows us to use strategies against specific arrays like in the current design.

[bebraw]

I have a solution at #150. Can you check and comment? Thanks!

[bebraw]

You should be able to do this type of merge in 5.2.0. 👍

[muuvmuuv]

Great, Thank you! Sorry for not responding but somehow my browser just crashed every time I have loaded this issue. All others work fine just on this one it just freazes. Pretty strange but seemed to be fixed from GH or Chromium.

[bebraw]

@muuvmuuv No probs. Let me know if you have any further issues. I think the current API is fairly powerful and it should allow most of what smart merging did but in an explicit manner (less magic).