andywer / webpack-blocks

📦 Configure webpack using functional feature blocks.
MIT License
2.97k stars 94 forks source link

The new block API #277

Closed vlad-zhukov closed 5 years ago

vlad-zhukov commented 6 years ago

I've proposed this API almost a year ago in #125 and I believe everyone is fine with it by now. TLDR:

// Current:

function postConfig (context, util) {
  const ruleConfig = Object.assign({
    test: /\.(js|jsx)$/,
    exclude: /node_modules/,
    use: [
      { loader: 'babel-loader', options: context.babel }
    ]
  }, context.match)

  return util.addLoader(ruleConfig)
}

// This PR:

function postConfig (context) {
  const ruleConfig = Object.assign({
    test: /\.(js|jsx)$/,
    exclude: /node_modules/,
    use: [
      { loader: 'babel-loader', options: context.babel }
    ]
  }, context.match)

  return context.addLoader(ruleConfig)
}

The #259 is different because it merges context and utils into an object:

function postConfig ({context, addLoader}) {
  // ...
}

cc @andywer @zcei @dmitmel @jvanbruegge

jvanbruegge commented 6 years ago

Not sure what this is solving actually. AFAICT 2.0 allows old blocks to be used with more or less no change at all, so not sure if this is worth a breaking change

zcei commented 6 years ago

So #259 and this PR are two competing approaches?

Same problem as @jvanbruegge, the mentioned discussion is too long and I don't recall all the details. Would you mind a small explanation what the problems are with the current API & what the up & downsides of the two competing PRs are?

Otherwise we can't make a profound decision, and I think some visibility helps when we later come back here (as it's linked in the changelog)

vlad-zhukov commented 6 years ago

Good point! However I'm unsure v1 blocks will work with v2 out of the box w/o any changes. Webpack 4 made breaking changes for loaders and in order to use old blocks with v2 a developer would need to update dependencies at the very least. Making another single line change shouldn't be a big of a problem, is it?

The reason for the change is purely stylistic. Some people (including me) would like to be it that way. There is also a rare case when a block doesn't use the context but needs a util function: (_, util) => util.addLoader({}).

There are no hard feelings about the change. How often do people develop new blocks anyway? 🙃

andywer commented 6 years ago

I think there was even a third option: Don't pass the util stuff (addLoader & friends) into the block at all, but rather make the blocks import them from a separate package.

(Sorry for the late reply!)

vlad-zhukov commented 6 years ago

@andywer Was it even considered as an option? 🙉 Right now I'm very close to leaving the API as it is for now. Maybe next year...

andywer commented 6 years ago

@vlad-zhukov Not sure how serious the efforts in that direction where, but I remember that one, too. And I totally get how you are feeling right now; this API discussion is a hornet's nest... 🙈

At some point I just stopped caring about it this API issue too much. Bottom line: No matter what we do here, the average webpack-blocks user will neither notice nor care 😜

vlad-zhukov commented 6 years ago

@andywer I don't care much about this change as I pointed above. I am fine with the discussion and I'm glad we've got people in our little community who also care about the project and can spot potential problems I wasn't aware of! 🍪

This is almost internal API and is no even near to day-to-day usage (more like year-to-year, huh 😁). As I said, the change is purely stylistic and if there are even little objections the change should no go further.

dmitmel commented 6 years ago

@andywer Oh, yeah! So we can move block utils into a separate module, and then instead of passing actual utilities to the block creators, we can pass a slightly modified version of them that print deprecation warnings.

andywer commented 6 years ago

@dmitmel No need to be snappish. I am just trying to recall the discussion at the time. And yes, I had my reservations as well! Nevertheless it doesn't hurt to gather the whole picture.

dmitmel commented 6 years ago

@andywer Ok, but I'm just trying to do something because I wasn't contributing to webpack-blocks for a long time.

andywer commented 6 years ago

@dmitmel Trust me, your efforts are highly appreciated! It's just a tricky change that requires quite a lot of communication. If you want, we can all have a chat tomorrow on gitter.im about the open issues and maybe we can find a less controversial issue that you can help with 🙂

But enough of that now. All non-PR-related talk moves to gitter now.

andywer commented 5 years ago

I'm sorry, but I don't see this happening at the moment. Closing for now.

Feel free to bring it up again 😉