Skip to content
This repository has been archived by the owner on Mar 16, 2020. It is now read-only.

fix(index): tapable deprecation warnings (webpack >= v4.0.0) #81

Merged
merged 2 commits into from
Mar 16, 2018

Conversation

ryanclark
Copy link
Contributor

@ryanclark ryanclark commented Mar 8, 2018

Notable Changes

Adds support for webpack >= 4.

@ryanclark ryanclark force-pushed the webpack-4 branch 2 times, most recently from e255f0a to 1f278a7 Compare March 8, 2018 21:11
@michael-ciniawsky michael-ciniawsky changed the title feat: add webpack 4 support fix(index): tapable deprecation warnings (webpack >= v4.0.0) Mar 8, 2018
package.json Outdated
"webpack": "^3.3.0",
"webpack-defaults": "^1.5.0"
"webpack": "^3.10.0",
"webpack-defaults": "^2.0.0-rc.2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do this in a separate PR, since webpack-defaults >= v2.0.0 requires node >= v6.0.0 and applying it is a semver major

src/index.js Outdated
@@ -8,6 +8,58 @@ function getDefault(actualValue, defaultValue) {
return actualValue !== void 0 ? actualValue : defaultValue;
}

const NAMESPACE = 'BabelMinifyPlugin';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this into the ctor of BabelMinifyPlugin

class BabelMinifyPlugin {
   constructor () {
      this.plugin = { name: 'BabelMinifyPlugin' }
   }
}

src/index.js Outdated
compilation.assets[file] = asset.__babelminified;
return;
}
const isWebpack4 = !!compiler.hooks;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this check is only used once if (compiler.hooks) is sufficient

src/index.js Outdated
let input;
let inputSourceMap;
if (isWebpack4) {
compiler.hooks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (compiler.hooks)
  const { plugin } = this
  const { compilation } = compiler.hooks
  
  compilation.tap(plugin, ...)
  ...
}

src/index.js Outdated
if (isWebpack4) {
compiler.hooks
.compilation
.tap(NAMESPACE, (compilation) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compilation callbacks could also be extracted into a {Function}

function buildModuleFn () {}
function optimizeChunkAssetsFn () {}

function compilationFn (compilation) {
   if (compilation.hooks) {
      const { plugin } = this
      const { buildModule, optimizeChunksAssets } = compilation.hooks
      
      buildModule.tap(plugin, buildModuleFn)
      optiimizeChunksAssets.tap(plugin, optimizeChunkAssetsFn)
   } else {
      compilation.plugin('build-module', buildModuleFn)
      compilation.plugin('optimize-chunk-assets', optimizeChunkAssetsFn)
   }
}

if (compiler.hooks) {
   ...
   compilation.tap(plugin, compilationFn)
} else {
   compiler.plugin('compilation', compilationFn)
}

package.json Outdated
@@ -29,10 +29,11 @@
"travis:coverage": "npm run test:coverage -- --runInBand",
"travis:lint": "npm run lint && npm run security",
"travis:test": "npm run test -- --runInBand",
"webpack-defaults": "webpack-defaults"
"webpack-defaults": "webpack-defaults",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- "webpack-defaults": "webpack-defaults",

package.json Outdated
@@ -28,9 +28,7 @@
"test:watch": "jest --watch",
"travis:coverage": "npm run test:coverage -- --runInBand",
"travis:lint": "npm run lint && npm run security",
"travis:test": "npm run test -- --runInBand",
"webpack-defaults": "webpack-defaults",
"defaults": "webpack-defaults"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"defaults": "webpack-defaults" should stay :P

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/index.js Outdated
function compilationFn(useHooks, compilation) {
const { options, plugin } = this;

if (useHooks) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useHooks => if (compilation.hooks)

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Mar 16, 2018

Release blocked by https://status.npmjs.org/incidents/gbk5bjx1169j 😞

}
});
if (compiler.hooks) {
const { compilation } = compiler.hooks;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In webpack we banned this pattern because it prevents searching for hooks.*.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... 🤔 debatable imho :P, personally always in favor of compacter code and here searching for the hookName (e.g compilation) instead then. A search for hooks*, one still gets where the hooks got destructured. Maybe destructuring is more confusing then helpful within larger parts of the webpack code base e.g Parser or the like ¯_(ツ)_/¯. Or did you mean something entirely different, with can break here ? 🙃

Anyways if there is general consensus on avoiding to destructure .hooks fine with either... :)

@michael-ciniawsky
Copy link
Member

Released in v0.3.1 🎉 Thx

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants