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

The new block API #277

Closed
wants to merge 2 commits into from
Closed

Conversation

vlad-zhukov
Copy link
Collaborator

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

@vlad-zhukov vlad-zhukov added this to the 2.0.0 milestone May 19, 2018
@vlad-zhukov vlad-zhukov requested review from zcei and andywer May 19, 2018 14:02
@jvanbruegge
Copy link
Contributor

jvanbruegge commented May 19, 2018

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
Copy link
Collaborator

zcei commented May 19, 2018

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
Copy link
Collaborator Author

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
Copy link
Owner

andywer commented May 22, 2018

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
Copy link
Collaborator Author

@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
Copy link
Owner

andywer commented May 22, 2018

@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
Copy link
Collaborator Author

@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
Copy link
Contributor

dmitmel commented May 22, 2018

@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
Copy link
Owner

andywer commented May 22, 2018

@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
Copy link
Contributor

dmitmel commented May 22, 2018

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

@andywer
Copy link
Owner

andywer commented May 22, 2018

@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
Copy link
Owner

andywer commented Jan 24, 2019

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

Feel free to bring it up again 😉

@andywer andywer closed this Jan 24, 2019
@andywer andywer mentioned this pull request Jan 24, 2019
@vlad-zhukov vlad-zhukov deleted the new-block-api branch January 24, 2019 07:01
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

Successfully merging this pull request may close these issues.

5 participants