API for version 1.0

[andywer]

After thinking a lot about it quite a long time, encountering several edge-cases with the current API and a long discussion with @aaronjensen I want to present the draft for a slight API change.

It is a breaking change, but migration is straight-forward. The version bump 0.x => 1.0/2.0 seems to be an obvious choice for introducing such a change.

Old (current) API

type Block = (context, prevConfig) => configSnippet

Block written for old (current) API

module.exports = function exampleBlock () {
  return (context, prevConfig) => ({
    module: {
      loaders: [
        {
          test: context.fileType('text/css'),
          loaders: ['sample-css-loader']
        }
      ]
    }
  })
}

The main issues here are:

• Implicit merge of the returned snippet
• No control over how this config snippet will be merged

(Pre and post hooks work as before, just that the post hooks will adapt the new API as well. Pre hooks don't actually emit configuration, so they don't need to be changed.)

New API

type Block = (context, helperFunctions) => prevConfig => nextConfig

Block written for new API

// With `merge()` helper:

module.exports = function exampleBlock () {
  return (context, { merge }) => merge({
    module: {
      rules: [
        {
          test: context.fileType('text/css'),
          use: ['sample-css-loader']
        }
      ]
    }
  })
}

// With `addLoader()` helper:

module.exports = function exampleBlock () {
  return (context, { addLoader }) => addLoader({
    test: context.fileType('text/css'),
    use: ['sample-css-loader']
  })
}

// Without any helpers:

module.exports = function exampleBlock () {
  return context => config => ({
    ...config,
    module: {
      ...config.module,
      rules: config.module.rules.concat([
        {
          test: context.fileType('text/css'),
          use: ['sample-css-loader']
        }
      ])
    }
  })
}

With the new API the modification of the configuration object is not just more explicit, but the main feature is that the block now has full control on how to merge new content into the configuration, like if some item will be prepended or appended to an array.

List of helper functions (Might still be subject to change)

• merge(configSnippet: object)
• addLoader(loaderDefinition: object)
• addPlugin(pluginInstance: Plugin)

@eXon @jvanbruegge @sapegin

[andywer]

FYI: This is pretty much the result of the #34 discussion 😉

[jvanbruegge]

I think this is a good change. It allows the extract text plugin to be used with any css preprocessor, because in a post hook, you can search the config for all loaders containing the style loader and use its mime type for the extract text plugin

[jvanbruegge]

Plus it keeps simple blocks as simple as they used to be because of merge

[eXon]

@andywer I love the idea of making what's going on more explicit. What I'm not 100% convinced though is why do we need a util parameter? If we are truly functional, wouldn't make sense to import those helpers instead?

[jvanbruegge]

With the parameter you dont have to update tge blocks if the core changes

[andywer]

@eXon Hmm. I don't think it contradicts the functional approach. My main argument for the helpers object is just preventing the blocks from all depending on the same util package. It should just cover the very most frequently used functions, so it's still very easy to write a block as a function without additional dependencies :)

[aaronjensen]

What I'm not 100% convinced though is why do we need a util parameter? If we are truly functional, wouldn't make sense to import those helpers instead?

Like Andy, I don't think that this has a bearing on whether or not the api is functional, but after thinking about it some... what it does do is make a dependency implicit that should probably be explicit.

There are problems around versioning something like this. Specifically, if a new util function is added and a 3rd party block uses it, then that block now has an implicit dependency upon that specific version of webpack-blocks. It would have to specify a peerDependency to make that explicit, but that is not forced and may be missed. Of course any other change to the util functions carry the same problem--parameter changing/renaming/etc. In other words, there is still a dependency, it's just implicit and not required. It's the opposite of "pit of success"

There is also the unit testing issue discussed here. The fact that these functions are injected led to testing them in an incomplete way. To test them in a complete way requires more work than it should.

I would propose a core package (maybe just called webpack-blocks?) that has the createConfig and utility functions exported. It would need to be webpack version specific (because it's been chosen to implement things like addLoader/addPlugin as utility functions). So, at that point, maybe it should also contain the @webpack-blocks/webpack blocks/code.

All blocks then would depend (probably still via peerDependency) on webpack-blocks and could use its functions for building blocks and testing.

While passing utility functions in is "easier", I think that making those dependencies explicit is "simpler".

[andywer]

@aaronjensen

I would propose a core package (maybe just called webpack-blocks?) that has the createConfig and utility functions exported.

I understand your implicity concern, but in terms of block development convenience I would still favor the passed-down utility methods. A block should be really simple to write and ideally be zero-dependency stuff.

If you need to do a complicated config manipulation you can import some external functions to help you at any time. But I really think you shouldn't have to for the usual cases.

Maybe it's a lot of personal flavor, but I always adored the cleanness of flummox and later redux: There was not a single import necessary; a completely transparent API. This embraces writing pure functions a lot and I would like to go a similar way here.

All blocks then would depend (probably still via peerDependency) on webpack-blocks and could use its functions for building blocks and testing.

There is always a subtle dependency between the block that exports a function implementing some interface and the createConfig() that must be able to consume it, even if the API is transparent (like no imports in the block).

We can make that dependency explicit by adding webpack-blocks/core/webpack-blocks/webpack as a peerDependency of the block when having the utility functions as well.

But I don't see an advantage when making the blocks import the utility functions. The dependency is basically the same as when receiving it via a parameter, the way of getting it is just different.

And I fear that other developers might accidentally add them as dependencies, not peerDependencies, for instance, and two version bumps later people might end up with multiple webpack-blocks/core of different version in their node_modules tree. This might lead to really hard-to-track issues...

[andywer]

Sorry for the wall of text 🙈😉

[jvanbruegge]

Im on Andy's side here, blocks shouldn't need depenencies

[aaronjensen]

But I don't see an advantage when making the blocks import the utility functions. The dependency is basically the same as when receiving it via a parameter, the way of getting it is just different.

My concern was around versioning. If you have 2 versions of webpack-blocks in the wild, one of which has the foo utility function and one doesn't, then any 3rd party blocks that use that foo function are only usable with the versions of webpack-blocks that have that function. End users will likely have no idea why they're getting foo is not a function errors if they're using an older version of webpack-blocks with this 3rd party block.

A bug report on the 3rd party block may lead them to adding a peerDependency on the specific version of webpack-blocks they depend on. This would be a good thing and may save others from stubbing their toe in the same way.

This gets even worse when considering the impact of updating webpack-merge. We've discussed possibly using merge instead of merge.smart. If that change were made after the release of 2.0, you may be breaking 3rd party blocks. (This, by the way, is another argument against mocking in block tests).

webpack-merge is about to rewrite and change the algorithm. Hopefully this won't have an impact on 3rd party blocks, but it may. And if you were to update to it and 3rd party blocks then relied upon the specific merge behavior, then, again, you'd have the situation as above with even more subtle consequences.

A block should be really simple to write and ideally be zero-dependency stuff.

Sorry, but inverting dependencies does not make the dependencies go away. They're still there. Blocks still depend on webpack-merge, they just don't know it or have the ability to specify which version they're going to get.

I'm probably pedantic here. No worlds will end as a result of the bugs/frustration this could potentially cause.

By the way, I think I was mistaken in my last post. If we were to force end users to require the utility functions, they should be declaring dependencies, not peerDependency. In that case the result is even better, they're guaranteed the version of utility functions/webpack-merge that they rely upon and have been tested with.

[jvanbruegge]

Ok, yeah that makes sense

[aaronjensen]

One other suggestion:

Put all the utils in their own package, webpack-blocks-utils or the like. It'd be small, be the only thing w/ a transitive dependency to webpack-merge. Blocks (built-in and 3rd party) would depend on that if they needed it (which they probably would for merge).

[andywer]

My concern was around versioning.

My solution was about versioning ^^
I would say that these problems can already be solved by sticking to semver. Utility functions can only be removed on a major version bump, new functions can only be introduced by at least a minor version bump.

Updating the webpack-merge dependency works the same. It requires at least a minor version bump.

As long as everyone sticks to semantic versioning it will be fine. If not we are doomed anyway, I fear.

Tell me if I am making things too easy here... 😉

Put all the utils in their own package, webpack-blocks-utils or the like.

Right, if that solution then definitely as a separate package :)

[aaronjensen]

I would say that these problems can already be solved by sticking to semver. Utility functions can only be removed on a major version bump, new functions can only be introduced by at least a minor version bump.

Sorry, I must be missing something. From what I can tell, semver/versioning have no impact on what you have proposed. You can bump 5 major versions and a 3rd party block that does not have a dependency or a peerDependency on @webpack-blocks/core won't care and it will break.

In order for semver/versioning to matter, packages must depend directly (or transitively) on the packages being versioned. Your proposal makes that unnecessary, so people won't do it.

My proposal was to make it necessary by forcing them to actually depend on the package that they depend on explicitly.

Am I missing something?

[andywer]

No, I meant in combination with peerDependencies 😉

But yeah, you have a point: peerDependencies rather feel like recommandations. They cannot enforce anything really...

I am still not liking the idea that pretty much every block will have to add this utils dependency.

My gut says "pass them as parameter, limit them to the absolute minimum functionality and only change them on major version bumps when breaking changes will make updating the blocks necessary anyway".

But you also have a point in making that dependency explicit. I guess I am too close to the issue right now, need to get a little distance first.

[aaronjensen]

At the end of the day, I don't think it is a big deal either way. It's unlikely to really cost anyone that much time and very unlikely to cause production issues. I'd be totally happy with it being however you like.

[andywer]

Ok, let's do it like this:

We are still some PRs away from the release and there is a working PR including the utility functions now (#128). Let's use it as it is for the moment and we still have time left to open another PR changing it if we want.

Agree? :)

[aaronjensen]

Works for me!

By the way, webpack-merge 4.0 was released which includes a patch that may fix the original issue with extract text. Whether or not it fully fixes it actually depends on whether or not the extract text plugin loader includes the fallback loader in its loader chain. Any way, it's a better behavior for us, so I'd suggest upgrading as part of 2.0

[andywer]

Thanks for the link. It's a useful change, but I don't think it will solve the extract-text issue.

It will improve the order of the loaders when merging definitions sharing the same loaders, but in the extract-text case we need to remove the loaders and add a new one...

[aaronjensen]

It will improve the order of the loaders when merging definitions sharing the same loaders, but in the extract-text case we need to remove the loaders and add a new one...

I thought so too, but it turns out that plugin.extract actually returns an array of loaders with an extract text plugin loader prepended to the list of loaders you provide it (which apparently does include the fallback loader, so I believe it would work. Of course, relying on that depends on implementation details of ExtractTextPlugin so it may not be a good idea. Ultimately, you're right, what we want to do is replace it outright.

[andywer]

Ahh, but that is interesting.

[jvanbruegge]

After thinking about this some more, i have to agree with @aaronjensen here.
Having explicit dependencies allows for decoupled development of the utils. New helpers can be added without updating the core.
In addition, some blocks might not need the utils, because their use case is more complex or they are using a different solution.
Lastly I often had problems with conflicting peer dependencies (different blocks depend on a different core) which would not be a problem with a normal (versioned) dependency.

But in the end the difference is minimal

[marcofugaro]

Nice job guys!

My only issue with it is that most of the times you wouldn't need the context, so it would be ideal if it was optional, and the first argument would be a big object containing context, merge, addLoader and so on...

module.exports = function resolveSrc() {
  return ({ merge }) => merge({
    resolve: {
      modules: ['node_modules', 'src'],
    },
  })
}

What do you guys think?

[andywer]

Hey @marcofugaro. This is indeed a good catch.

@aaronjensen Do you guys already use the new API / the release branch?

[andywer]

I'd say globs are user-friendlier and easier on the eyes for matching file types. Like *.css seems friendlier than /\.css$/. Bash-like globs like {*.js, *.jsx} should be powerful enough for pretty much every use case you might encounter, yet easily understandable.

I first thought about using minimatch, but glob-to-regexp seems to be a better choice for turning globs into regexps. (I tried both)

[andywer]

I have a first working prototype. It's work in progress, but the diff of the @webpack-blocks/assets README provides a pretty neat foretaste I think.

Feedback is as always highly appreciated, though 😉

[sapegin]

I like the API! ;-)

[andywer]

(In case you didn't see it: The PR is open - #163 😉)

[tribou]

@andywer What is the support for Webpack 3 going to look like? Or is that being considered in this release?

[andywer]

Webpack 3 comes with no breaking API changes that would concern webpack-blocks, so I am just adding it to #134.

That reminds me... Right now @webpack-blocks/webpack has webpack set as a dependency. We should turn that into a peerDependency, so the user can decide which version of webpack they want to use!

[vlad-zhukov]

I had a thought the other day why are helper functions passed as a separate argument? They should be a part of the context, aren't they?

[andywer]

@vlad-zhukov Fair enough. The original line of thought goes like this:

The context is a thing where data can be stored that should be shared by multiple blocks or between hooks. Since the helper functions are helper functions and not shared data, they are passed separately.

But of course you can argue that it's just a question of re-defining the purpose of the context 😉

And yeah, we still need to settle on the future of those helper functions... 🙃

[vlad-zhukov]

I see but fileType is a helper function too, and it's in the context since the first days.
And webpack as well.

[andywer]

fileType is a getter and is gone in 1.0, webpack is just for accessing the webpack plugins.

But it doesn't always seem intuitive where to draw the line, so you are not wrong, though 😉

The main question right now is still:

• Should those helper functions be passed to the blocks
• Or should the blocks import the helper functions

There seems to be a small majority in favor of the 2nd option.

[andywer]

Important

Sooo. This is probably the longest running discussion of the project and there was a lot of valuable feedback, yet no complete consensus yet.

My suggestion:
Since there is still no 1.0 and introducing major breaking changes now would just further postpone the 1.0 release I propose that we leave the API for now as it is. We can open a separate issue to continue the discussion and eventually turn that into a webpack-blocks 2.0.

But for now I'd like to take the fastest route leading to a working 1.0, since 0.4 should be deprecated as soon as possible.

[sapegin]

Do it!

[andywer]

For mid-term consideration:

I slowly feel like changing my mind towards what @vlad-zhukov wrote earlier: Might make sense to put the utils (2nd parameter: addLoader & friends) in the context and have only one parameter passed to the blocks.

Main benefit: There will always be blocks that want the utils, but not the context, and blocks that want the context, but no utils. In the former case you currently end up with a block = (_, { merge }) => {} which isn't very nice. Having only one combined parameter would fix that.

[dmitmel]

@andywer I've created a PR (#259) that shows how the new block API will look like.

[andywer]

Thanks for your efforts, @dmitmel! 😊

But I would schedule #259 for after the 1.0 release. We should finally ship the v1.0 now, without further breaking changes. But let's tackle this right after 1.0.

[dmitmel]

@andywer I think that the old API should be completely removed in the 2.0 release.

[dmitmel]

@andywer I think it's better to put helper functions into block instance's options, i.e.:

  function css () {
-   return ({ context, util }) => util.addLoader({
+   return ({ context, addLoader }) => addLoader({
      test: /\.css$/,
      use: [ 'style-loader', 'css-loader' ],
      ...context.match
    })
  }

[andywer]

@dmitmel Maybe, but this is the wrong issue for that 😉

[dmitmel]

@andywer Well, this can be an API for v2.0, but it's better to introduce one breaking change than two.

[andywer]

v1.0 has been released.

[dmitmel]

@andywer Hooray!