New API #128

andywer · 2017-03-02T18:21:17Z

Introduces the new core API. Resolves #125.

Still using module.loaders and webpack-merge's smart merge, though.

@aaronjensen

Necessary to make it work with node 4.

andywer · 2017-03-02T18:23:31Z

@jvanbruegge @eXon Feel free to review as well 😉

jvanbruegge · 2017-03-02T18:30:33Z

packages/css-modules/index.js

-  })
+  return (context, util) => util.addLoader(
+    Object.assign({
+      test: context.fileType('text/css'),


Does this work with sass too?

Don't think so, but never tried it to be honest... Good point 🤔

If not, use a post hook to query all loaders with style loader and use that mime type

I opened a follow-up issue for this: #130.

jvanbruegge · 2017-03-02T18:33:26Z

packages/typescript/index.js

@@ -44,6 +42,7 @@ function typescript (options) {

 function pre (context) {
  const registeredTypes = context.fileType.all()
+
  if (!('application/x-typescript' in registeredTypes)) {


Would remove this pre hook, as it is now in the default mime types and the 2.0 update is a breaking one, so you have to update

aaronjensen · 2017-03-02T18:24:56Z

docs/BLOCK-CREATION.md

+  })
+}
+```
+


I'd add group to this list

aaronjensen · 2017-03-02T18:30:24Z

packages/babel6/__tests__/babel6.test.js

-  t.deepEqual(block(context), {})
-  t.deepEqual(block.post(context).module.loaders, [
+  t.deepEqual(block(context, { addLoader })({}), {})
+  t.deepEqual(block.post(context, { addLoader })({}), {})


addLoader is a pure function, so it's usually a better idea to include/use the real one and do state based testing rather than setting it up as a spy. In general, I'd consider pushing towards doing snapshot testing for blocks, but in the meantime at least using the real utility functions would make these tests more valuable imo.

Also, I'm not entirely convinced that unit testing blocks is worthwhile. You get more bang for the buck by actually testing them in createConfig, I think.

Yeah, I have got mixed feelings about unit testing the blocks, too. Integration tests are definitely worth more. But I didn't want to open another construction site right now...

The reason why I didn't use the real helpers was just that the unit test would then not only test this unit, but also the utils. Also using a spy makes testing quite easy.

But yeah, we should consider turning the block unit tests into integration tests.

The reason why I didn't use the real helpers was just that the unit test would then not only test this unit, but also the utils. Also using a spy makes testing quite easy.

Right, but it's misusing spies imo. Spies should only be used for side effecting things. These are pure functions. It's ok to "test" them, because what you're really testing is that they do what you think they're doing in this specific context. I wouldn't even consider it an integration test. You shouldn't have things that return values be spies, it's like replacing merge with a spy, or mapValues.

I don't think there's anything in webpack-blocks that warrants testing w/ sinon, there are no side effects. It's all pure functions which is part of its beauty.

Btw, this is a good video about spies vs mocks vs not using them https://www.youtube.com/watch?v=URSWYvyc42M

I wouldn't even consider it an integration test.

It's not an integration test, right now it's a unit test. But maybe we talk past each other here. It will probably be best to turn it into an integration test, using it in createConfig() and look at the resulting config.

Agree? Might do it in another PR, though.

Right, and it would still be a unit test even if it used the real util functions. That was my point. I'd urge you to remove sinon as a dependency and do that instead (another pull is fine) or port to a test that uses createConfig (and still remove sinon). I don't think sinon is a good fit for functional projects and I wouldn't want to encourage contributors to misuse it.

aaronjensen · 2017-03-02T18:32:10Z

packages/core/package.json

+    "babel": {
+      "plugins": [
+        [
+          "transform-object-rest-spread",


I thought ava included this by default? https://github.com/avajs/ava#es2017-support

I thought so, too. But it seems quite a lot of features that once were part of the es2017 preset have been removed from it recently. Object-spread isn't included anymore... :-/

https://babeljs.io/docs/plugins/preset-es2017/

Ah, indeed, it's in experimental.

aaronjensen · 2017-03-02T18:34:53Z

packages/core/src/index.js

-      const configPartial = setter(context, getCompleteConfig(mergedConfig))
-      return merge.smart(mergedConfig, configPartial)
+    (config, setter) => {
+      const updateFunction = setter(context, helpers)


helpers -> util

aaronjensen · 2017-03-02T18:36:33Z

packages/extract-text/index.js

+    // Now apply them to the config: Remove the existing loaders for that
+    // file type, add the new loader and add the plugin
+
+    return _addPlugin(_addLoader(_removeLoaders(prevConfig)))


good place to use group

But it isn't a group of blocks, it's just three update functions... I already thought about util.aggregate([ _removeLoaders, ... ]) or something like that, but I'm hesitating adding more new stuff to util.

I mentioned flow before, it's a lodash function. It's what you need here (though you probably don't need to pull it in from lodash unless you want to). And I'd say it's a must have. It's a key part of function composition which is what this api should drive people to.

I prefer my implementation of flow because it flattens and removes nils allowing you to compose like this:

flow( option && doThing(), option2 && [doThing1(), doThing2], doOtherThing(), )

Hmm, that might be a candidate to be included in util...

andywer · 2017-03-06T10:15:51Z

So as far as I can see there are no immediate issues left to fix for this PR.

One topic that might be considered part of this PR is:
Should we add a flow() helper as pointed out by @aaronjensen?

One topic that relates to this PR, but can be settled later:
Should the built-in utility functions be dropped and moved to a separate package? (#125)

andywer · 2017-03-06T21:31:56Z

Merged it, since the open topics don't block this PR.

Andy Wermke added 15 commits March 2, 2017 11:52

Implement new core API

1b973a6

Update core tests to match new API

f3658a5

Update babel6 block to use new API

700b20d

Update css-modules block to use new API

1d1fc1d

Update dev-server block to use new API

4152cf8

Update extract-text block to use new API

83eed3e

Update postcss block to use new API

799d06a

Update sass block to use new API

a564ad7

Update tslint block to use new API

11ee849

Update typescript block to use new API

095fe20

Update webpack package to use new API

a420319

Let's call the 2nd param util instead of helpers

0c02abf

Add @ava/babel-preset-stage-4 preset to devDeps

1d57ec5

Necessary to make it work with node 4.

Update changelogs

7498b16

Update docs to match new core API

5574f25

jvanbruegge reviewed Mar 2, 2017

View reviewed changes

aaronjensen reviewed Mar 2, 2017

View reviewed changes

Andy Wermke added 2 commits March 2, 2017 20:31

Drop typescript pre hook

ff52809

Rename helpers => blockUtils

3598f93

andywer force-pushed the feature/new-api branch from b952726 to 3598f93 Compare March 2, 2017 19:31

aaronjensen mentioned this pull request Mar 3, 2017

API for version 1.0 #125

Closed

aaronjensen mentioned this pull request Mar 6, 2017

Port to webpack 2.0 config style #124

Merged

andywer merged commit dc3f5b3 into release/2.0 Mar 6, 2017

andywer deleted the feature/new-api branch March 6, 2017 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New API #128

New API #128

andywer commented Mar 2, 2017

andywer commented Mar 2, 2017

jvanbruegge Mar 2, 2017

andywer Mar 2, 2017

jvanbruegge Mar 2, 2017

andywer Mar 6, 2017

jvanbruegge Mar 2, 2017

andywer Mar 2, 2017

aaronjensen Mar 2, 2017

aaronjensen Mar 2, 2017

andywer Mar 2, 2017

aaronjensen Mar 2, 2017

aaronjensen Mar 2, 2017

andywer Mar 2, 2017

aaronjensen Mar 2, 2017

aaronjensen Mar 2, 2017

andywer Mar 2, 2017

aaronjensen Mar 2, 2017

aaronjensen Mar 2, 2017

aaronjensen Mar 2, 2017

andywer Mar 2, 2017

aaronjensen Mar 2, 2017

andywer Mar 2, 2017

andywer commented Mar 6, 2017

andywer commented Mar 6, 2017

New API #128

New API #128

Conversation

andywer commented Mar 2, 2017

andywer commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andywer commented Mar 6, 2017

andywer commented Mar 6, 2017