webpack-merge or not?

[andywer]

Right now webpack-blocks is using webpack-merge to merge the blocks' config snippets to a complete config object.

There are some problems with webpack-merge, though:

• It is designed to concatenate/extend config objects/properties/arrays, but it is a problem to clear an object that's part of the webpack config that has been set before.
• An array might be cleared by passing an empty array to the merge function. But this causes another issue: You wouldn't expect that merging an empty array clears all existing items.

The webpack-merge makers are already discussing the issue (survivejs/webpack-merge#45), but I am not a big fan of the proposed approach.

Possible solution: Use https://github.com/kolodny/immutability-helper instead of webpack-merge altogether.

Open for discussion :)

[eXon]

Is webpack-merge doing anything special webpack related? If not that much, we could simply use lodash.merge. There is even a package for just that function called lodash.merge.

It's what I was using for Webpacker and it is working pretty well. Instead of doing merge.smart(config1, config2) you would do merge({}, config1, config2). Maybe it's worth a try :)

AFAIK merge.smart is doing the same thing + merging loaders if they have the same regex. Not sure it is much need and would probably be easy to replicate.

[andywer]

@eXon Yeah, merge.smart() does merge the loaders, the entry pointers and maybe some more stuff.

The problem about lodash.merge as I think is that you will again run into the issues of clearing an array or object that has been set before by applying a diff. Or just doing more complicated changes based on existing props.

With the immutability helpers you could do this:
{ plugins: { $push: [ ... ] } } (Add another plugin)
{ module: { loaders: { $apply: someFunction } } } (someFunction = (oldLoaders) => newLoaders)

[eXon]

I think we will need to normalize configs just like the entry point to make sure everything is merged correctly. There is no way around it.

I've been using immutable.js for some time so I understand there is some benefits to this.

[andywer]

I think using the immutability helpers would give us much more power and at the same time make the webpack config merging details more explicit. There are two ways I see right now:

The soft way

Having a simple heuristic to determine if a webpack config snippet is supposed to be handled by the immutability helper (by looking for $-prefixed command props). If not webpack-merge is used as before.

Pro:

• For simple scenarios you might prefer the current way over using the immutability helper
• Staying backwards compatible

Contra:

• Providing two ways to do things might increase complexity quite a lot (webpack-blocks code as well as learning curve if you want to write/debug a block)

The hard way

Dropping webpack-merge and focusing on the immutability helper.

Pro:

• One clear way to do stuff, not adding additional complexity

Contra:

• Might be a little harder to create simple webpack blocks
• Not backwards compatible, introduces a major breaking change

[eXon]

I don't really like either. The hard way might be better because I don't think there is a lot of blocks written ATM and a minor version bump could do the job.

However, I'm not sure if it's needed at all. After reading the code (didn't expected it to be that small), I'm not sure what are the problematic use-cases. The react-hot issue is fixed by updating the version and the original order is respected a lot better than before.

Care to give a few examples that are either problematic or anoying?

[willhoney7]

I'm not a big fan of the "hard" way. It forces complexity on the user... which isn't the point of webpack-blocks to reduce it?

The soft way is a solid compromise but I think it's overkill.

Another option could be to add another webpack-block export that is called clearConfig. It would then use something like lodash.unset to remove it. I'm not entirely confident that would work for what @andywer is requesting, but perhaps. It could also be a third party webpack block.

UPDATE: I noticed that webpack-merge v2.0 has merge.strategy (https://github.com/survivejs/webpack-merge#api) which allows the replacing of values. That could also work with a clearConfig style block.

At the end of typing that, I'm trying to understand why someone would want to clear a previous webpack config anyway? I think that means that specific webpack-block doesn't fit your needs.

[andywer]

The "hard way" would add a little more complexity, but just for the ones writing blocks, not for the ones using those blocks. So it's not that bad 😉

But it's not just about clearing properties, but also appending vs. prepending items to an array, for instance. This is quite important, since array config props are usually order-sensitive.

Yes, there is merge.strategy now, but a) we would have to change the block API in order to make the block decide how its returned config will be merged by createConfig() and b) I think that the merge.strategy approach is considerably less elegant than the immutability helpers.

[andywer]

This might also have an impact on #14.

Dropping loaders from the config and adding a plugin instance that includes those loaders is not feasible with webpack-merge.

[aaronjensen]

We're running into issues w/ the 2.0 extraction because of this.

Specifically, extract-text-plugin previously relied on moving from a compound loader to a loaders array. In webpack2, both of those are use and webpack-merge does not allow you to prepend loaders, which is what ExtractTextPlugin wants to do.

The approach that I took in webpack-parts worked quite well for stuff like this. In my case, I used the default merge. This makes sense because I wasn't going for as much composability in webpack-parts. For example, ExtractTextPlugin was just a part of css. If you wanted sass instead of postcss, I probably would have just had an option for that. So one part for css, that was more configurable. This allowed me to not have to worry about merging loaders at all and that simplified things.

Blocks takes a different appraoch though and goes for multiple blocks that compose (so you could have sass, postcss, less, extracttextplugin) all as separate blocks. This likely means some reliance upon merge.smart. Unfortunately, it breaks down.

So, when it breaks down, it is nice to be able to fall back to an updater function. The function would take the entire webpack config and return the new webpack config. It wouldn't need to be an all-or-nothing approach, in webpack-parts I handled both returning objects from parts (which were merged) and functions (which were invoked).

webpack-blocks complicates things a little further with its group functionality. I think that that entire functionality can be replaced by flattening the arguments to createConfig. In that way, you can just put groups of things in an array and pass those around.

So that means createConfig can take arguments of 3 types:

Arrays: flattened out
Objects: merged in to the webpack config (with merge.smart or merge, depending on how breaking you want the change to be. Personally, I prefer merge because it is less "surprising")
Functions: Invoked with the webpack config and are expected to return the new webpack config.

I handled a fourth, falsey things, which were filtered out. This allowed me to write parts like:

const hotModuleReloading = (
  {
    useReactHotLoader,
    useWebpackHotMiddleware,
    webpackDevServerUrl,
  } = {}
) => ifProd(
  null,
  [
    failIfNotConfigured('entry', 'hotModuleReloading'),
    useWebpackHotMiddleware &&
      prependToEachEntry('webpack-hot-middleware/client'),
    webpackDevServerUrl && prependToEachEntry('webpack/hot/only-dev-server'),
    webpackDevServerUrl &&
      prependToEachEntry(`webpack-dev-server/client?${webpackDevServerUrl}`),
    useReactHotLoader && prependToEachEntry('react-hot-loader/patch'),
    {
      plugins: [
        new webpack.HotModuleReplacementPlugin(),
        new webpack.NoEmitOnErrorsPlugin(),
      ],
    }
  ]
)

[andywer]

@aaronjensen Ohh yeah, that is an issue...

You already have some good points, but I want to further clarify a few things:

This likely means some reliance upon merge.smart. Unfortunately, it breaks down. So, when it breaks down, it is nice to be able to fall back to an updater function.

Yeah, right now I see two alternatives to webpack-merge:

1. returning a function (config) => config in the block (let the block handle the merging)
2. using the immutability-helper instead

The first approach could also allow a soft transition (we check if a block returns a config snippet or an update function), might be a good idea, but might also make things fuzzy... Gotta keep thinking about that.

I also fear that 2. makes blocks harder to read. Upside is, though: It makes them easier to debug, since a list of immutability-helper objects is easy to serialize, a list of functions is usually not.

webpack-blocks complicates things a little further with its group functionality. I think that that entire functionality can be replaced by flattening the arguments to createConfig.

I don't think this is really part of this discussion, but just for clarification:
group() returns a new block, so basically createConfig() just operates on an always-flat array already.

We can speak about anything beyong that later. For now I would like to focus on the webpack-merge part, esp. as it seems to be an urgent problem.

[aaronjensen]

The first approach could also allow a soft transition (we check if a block returns a config snippet or an update function), might be a good idea, but might also make things fuzzy...

Is the fuzziness you're referring to the fact that a block has two modes of operation now? Function vs object? I'd think of the object as a shortcut/convenience mechanism, but you could do away w/ that by providing a merge({ ... }) function that returns a function that will do the merge. That said, I don't think it's necessary. I doubt people will have a hard time knowing/understanding the difference between an object and a function. The large majority of blocks will work just fine w/ an object as they do today.

using the immutability-helper instead

I'd strongly advise against this, you'd just be coupling to another tool that may have its limitations. I could see us having a similar discussion some days/weeks after starting down that path. I think it's better to allow the block creator to make the tool choice. Personally, I'd use updeep if I needed something like immutability-helper, as I much prefer functions to magic keys.

It makes them easier to debug, since a list of immutability-helper objects is easy to serialize, a list of functions is usually not.

If debugging is necessary, printing the before/after (input/output) as well as the function name would be a fine experience IMO. I don't think seeing the intermediate objects would be super necessary to debug. Functions are pretty easy to test and work with.

I don't think this is really part of this discussion, but just for clarification:
group() returns a new block, so basically createConfig() just operates on an always-flat array already.

Sorry, I should have been more clear. I actually tried to implement allowing a function to be returned from a block and was stopped by the group functionality, which is why I brought it up. I agree though, we can work it out once the decision is made which way to go.

[andywer]

Is the fuzziness you're referring to the fact that a block has two modes of operation now?

Yeah. Having just one way to do it (functions only) seems cleaner, but having two might be more convenient, esp. if the function use-case is frequently not needed.
I think this is major point to decide here

I'd strongly advise against this (immutability helper)

Fair enough. I am leaning towards the update function as well.

Added a gist with a suggestion for a new API here: https://gist.github.com/andywer/4bdd9f4e5b3e6d4b94a1f4d843304dc4

[andywer]

Closing this issue, since 1.0 will allow the blocks to merge config as they like.