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

How to replace smart function when a specific loader in a chain needs to be overriden? #149

Closed
TorRanfelt opened this issue Sep 23, 2020 · 15 comments

Comments

@TorRanfelt
Copy link

The following works in webpack-merge 4.2.2, but the smart function has been removed.
What I want is to override the configuration of the css-loader.

const base = {
        module: {
            rules: [
                {
                    test: /\.scss$/,
                    loaders: [
                        {
                            loader: MiniCssExtractPlugin.loader
                        },
                        {
                            loader: 'css-loader'
                        },
                        {
                            loader: 'postcss-loader',
                            options: {
                                plugins: function () {
                                    return [
                                        projectRequire('autoprefixer')
                                    ];
                                }
                            }
                        },
                        {
                            loader: 'resolve-url-loader'
                        },
                        {
                            loader: 'sass-loader',
                            options: {
                                sourceMap: true
                            }
                        }
                    ]
                }
            ]
        }
    }

    const development = {
        module: {
            rules: [
                {
                    test: /\.scss$/,
                    loaders: [
                        {
                            loader: 'css-loader',
                            options: {
                                sourceMap: true
                            }
                        }
                    ]
                }
            ]
        }
    }
@bebraw
Copy link
Member

bebraw commented Sep 23, 2020

I think this one is related to #146. I have a potential proposal at #146 (comment) . Can you check and let me know if that would cover this case as well?

In you case, another option would be to do a composition like this:

const commonConfig = {... }; // Everything else but that css-loader bit from your current config
const productionConfig = {...}; // Only the css-loader portion from your `base`
const developmentConfig = {...}; // You current development config

// Then to compose
// Production
merge(commonConfig, productionConfig);

// Development
merge(commonConfig, developmentconfig);

I explain the composition approaches here. I believe in your case it may be better to move the css-loader bit out of base and change the composition.

@TorRanfelt
Copy link
Author

TorRanfelt commented Sep 24, 2020

Would mergeArrays allow this code example?
Also, can it be made simpler; it seems cumbersome? - The only documentation I was able to find was the one found on the github and npm front-pages, and I wasn't able to make it simpler from that.

I assume sophisticated merge of loaders in rules is a common need. Maybe not something everybody should make on their own.

const WebpackMerge = projectRequire("webpack-merge");

WebpackMerge.mergeWithCustomize({
    customizeArray(rules1, rules2, key) {
        if (key === "module.rules") {
            return WebpackMerge.mergeArrays(
                'test',
                (rule1, rule2) => ({
                    return WebpackMerge.mergeWithCustomize({
                        customizeArray(loaders1, loaders2, key) {
                            if (key === "loaders") {
                                return WebpackMerge.mergeArray(
                                    'loader',
                                    (loader1, loader2) => ({
                                        return WebpackMerge.merge(loader1, loader2)
                                    }) (loader1, loader2)
                                )
                            }
                        }
                    }) (rule1, rule2)
                })
            )(rules1, rules2)
        }
    }
})(base , development);

@bebraw
Copy link
Member

bebraw commented Sep 24, 2020

@TorRanfelt That's exactly what I was thinking about at #146 (comment) .

I could provide a couple of building blocks like in the code example at my comment.

Here's the code for reference:

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

@TorRanfelt
Copy link
Author

TorRanfelt commented Sep 24, 2020

I read your comment, but I was unsure how it would work in its entirety. Hence my example in order to verify that I had understood what you meant. - Your example is much simpler.

The strategy can be simplified to this when using "..." and the fact that a rule-object only have one array ('loaders'):

WebpackMerge.mergeWithCustomize({
  customizeArray(rules1, rules2, key) {
      if (key === "module.rules") {
          return WebpackMerge.mergeArrays(
              'test',
              (rule1, rule2) => ({
                    ...merge(rule1, rule2),
                    use: mergeArrays('loader', merge)
                  })
              })
          )(rules1, rules2)
      }
  }
})(base , development);

Is this correct?
Can it be made even simpler?

@bebraw
Copy link
Member

bebraw commented Sep 24, 2020

@TorRanfelt With my proposed API and your problem, I think it would fall into 1. Check against test 2. Check against loader 3. Merge if both match.

Based on that, I imagine it would look like this:

mergeArrays(
  'test',
  (a, b) => ({
    ...merge(a, b),
    // Now the last with the same loader name would override completely.
    // I think mergeArrays should give enough flexibility to be able to customize more.
    loaders: unique('loaders', 'loader')(a, b)
  })
)(a, b)

Apart from the ... per item to merge within mergeArrays I sort of like the approach. Maybe the ... can go away if we call merge there. Then you start getting patterns like mergeArrays('test', mergeWithCustomize({ ... }).

The key to this work is to find good building blocks for the API so we can both match and merge within a match and I imagine the final API will go to this direction.

What you had in your code example would likely work as an API but it would be nice to find something that's concise and somehow readable.

@TorRanfelt
Copy link
Author

TorRanfelt commented Sep 25, 2020

@bebraw Maybe an API that allows the following:

WebpackMerge.mergeWithCustomize({
  customizeArray("module.rules") {
      WebpackMerge.mergeArrays(
              'test',
              WebpackMerge.mergeWithCustomize({
                customizeArray("loaders") {
                  mergeArrays('loader', merge)
                }
              })
          )
})(base , development);

@bebraw
Copy link
Member

bebraw commented Sep 25, 2020

@TorRanfelt Yeah, that looks good. I'll try to implement something along those lines as doing this the right way should solve a couple of issues at once.

@bebraw
Copy link
Member

bebraw commented Sep 25, 2020

@TorRanfelt I played around with the implementation. Here's the proposed API: #146 (comment) .

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

It's going to need a small parser (parser generator even?) but I can map it from the selector syntax to code.

@bebraw
Copy link
Member

bebraw commented Sep 28, 2020

I started implementing the chaining based API. I realized below is even more straight-forward:

mergeWithCustomize({
  customizeArray: customizeArray({
    module: {
      rules: {
        test: CustomizeRule.Match,
        loaders: CustomizeRule.Replace
      }
    }
  })
})

This form has the added benefit of flexibility within a match and it seems easier to implement as well.

@bebraw
Copy link
Member

bebraw commented Oct 1, 2020

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

@TorRanfelt
Copy link
Author

@bebraw
In order to meaningfully comment on the commits I would need to delve into the existing code-base.
However, the API you have distilled is beautiful.

@bebraw
Copy link
Member

bebraw commented Oct 5, 2020

@TorRanfelt Ok, great to hear you like the API. I was thinking maybe it should replace the current customizeArray/customizeObject bit entirely in a future major version. It's good enough for a feature release to gain further feedback.

The only problematic part I can see with the design is that given webpack configuration is polymorphic (esp. for loaders), it's not going to work for configurations that mean the same but have different structure in them. There isn't much I can do about that, though. The upcoming setup is close to as good as I can make it. 😄

@TorRanfelt
Copy link
Author

@bebraw I don't think that's a problem. It's a limitation that is easy to understand, and it should be easy to keep the structure identical wherever a merge is needed (personally I think a consistent structure is the way to go anyway).

Offering only merge of configurations on a syntactical level and not a semantic level is a sane scope constraint. The semantic level would yield negligible and frankly unnecessary gains while the work would be huge, error-prone and would require changes as webpack's semantics changes.

@bebraw
Copy link
Member

bebraw commented Oct 5, 2020

Offering only merge of configurations on a syntactical level and not a semantic level is a sane scope constraint. The semantic level would yield negligible and frankly unnecessary gains while the work would be huge, error-prone and would require changes as webpack's semantics changes.

Ok, makes sense. 👍

There's one corner case I have to tackle. After that the PR is good to merge.

@bebraw
Copy link
Member

bebraw commented Oct 7, 2020

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

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

2 participants