feat: add webpack v2 support #226

jwhitmarsh · 2017-10-10T13:18:53Z

I've updated the special/webpack.js file to parse new webpack v2 (and greater) config better, and added some tests to support the new functionality.

lijunle

@jwhitmarsh , thank you very much for working on this. I have some comments on the implementation. Could you please update the code? Thank you again!

lijunle · 2017-10-10T13:41:00Z

src/special/webpack.js

+
+    return getLoaders(deps, lodash.flatten(mappedLoaders));
+  } catch (err) {
+    return console.error(err);


This could pollute the logs. I am sorry that, we don't have a nice way to control the logs (maybe debug package). But now, could better just skip it silently.

lijunle · 2017-10-10T13:53:13Z

src/special/webpack.js

-    const postLoaders = getLoaders(deps, module.postLoaders);
-    return loaders.concat(preLoaders).concat(postLoaders);
+    const webpack1Loaders = parseWebpack1(filepath, deps);
+    const webpack2Loaders = parseWebpack2(filepath, deps);


Why not pass module for parseWebpack1/2? I only see one usage on filepath variable.

lijunle · 2017-10-10T14:07:01Z

src/special/webpack.js

+    .map((rule) => {
+      const key = rule.use ? 'use' : 'loader';
+
+      // map simple strings as { loader: string }


Instead of having three if clauses, I could suggest to convert the first two cases to Array, then handle it with the third case logic.

lijunle · 2017-10-10T14:08:38Z

src/special/webpack.js

+  return module.rules
+    .filter(rule => rule.loaders)
+    .map(rule => rule.loaders
+      .map(loader => ({


Could better to inline:

.map(rule => rule.loaders.map(loader => ({ loader })));

lijunle · 2017-10-10T14:09:23Z

src/special/webpack.js

+      return [];
+    }
+
+    const mappedLoaders = [];


Directly construct mappedLoaders with [...mapRuleLoader(), ...mapRuleUse()].

lijunle · 2017-10-10T14:10:20Z

src/special/webpack.js

+    mappedLoaders.push(...mapRuleLoaders(module));
+    mappedLoaders.push(...mapRuleUse(module));
+
+    return getLoaders(deps, lodash.flatten(mappedLoaders));


Is this lodash.flatten necessary? I think you have good structure at this moment.

It is - mapRuleUse can return an array of arrays because use or loader can be an array. Eg:

module: { rules: [ { test: /\.css$/, loader: [{ loader: 'style-loader' }] }, ], },

Rather than test the type of each loader or use property it's easier to just flatten the whole lot.

I've made the rest of the changes - thank you for your excellent feedback! I'll wait until we've worked out what's going on here ^ before resubmitting the PR.

codecov · 2017-10-10T15:32:51Z

Codecov Report

Merging #226 into master will increase coverage by 0.28%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   98.15%   98.43%   +0.28%     
==========================================
  Files          25       25              
  Lines         433      447      +14     
==========================================
+ Hits          425      440      +15     
+ Misses          8        7       -1

Impacted Files	Coverage Δ
src/special/webpack.js	`100% <100%> (+4.16%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 842959d...5a8113b. Read the comment docs.

jwhitmarsh · 2017-10-10T15:42:15Z

src/special/webpack.js

-      })));
+    .map(rule => rule.loaders.map(loader => ({
+      loader,
+    })));


Sorry, I did change this but it looks like it's auto formatted back to this. I'll change it again.

jwhitmarsh · 2017-10-10T15:43:44Z

Apologies - I forgot that pushing to the branch would auto trigger this update (I thought i'd have to manually re-submit the PR).

Not sure why codecov is now failing?

lijunle

Overall looks good, left some small feedback.

lijunle · 2017-10-10T17:13:03Z

src/special/webpack.js

@@ -1,12 +1,16 @@
+/* eslint-disable no-console */


I don't think we need this disable rule anymore.

lijunle · 2017-10-10T17:15:11Z

src/special/webpack.js

+    return [];
+  }
+
+  return getLoaders(deps, lodash.flatten([...mapRuleLoaders(module), ...mapRuleUse(module)]));


Directly return a complicated computation expression is not a good idea. It will make it hard to debug (inspect the return value). Split it to some small steps, it could make debugging easier.

lijunle · 2017-10-10T17:18:44Z

src/special/webpack.js

 import path from 'path';
 import lodash from 'lodash';

 const webpackConfigRegex = /webpack(\..+)?\.config\.(babel\.)?js/;
 const loaderTemplates = ['*-webpack-loader', '*-web-loader', '*-loader', '*'];

 function extractLoaders(item) {
-  if (item.loader) {
+  if (item.loader && typeof item.loader === 'string') {


I am thinking about if we can check item is a string, then directly return it. If that is possible, we don't need to have .map(loader => ({ lodaer }))

lijunle · 2017-10-10T17:19:49Z

src/special/webpack.js

+      const key = rule.use ? 'use' : 'loader';
+      const coercedArray = [].concat(rule[key]);
+
+      // if it's an array, apply the two rules above for each element


The comment has been outdated.

lijunle · 2017-10-10T17:20:57Z

src/special/webpack.js

+        }
+
+        return {
+          loader: value,


If we check typeof item === 'string' in extractLoaders, this could be simplify too.

lijunle · 2017-10-10T17:24:25Z

Thank you @jwhitmarsh for updating the PR.

The codecov is failing because the coverage is dropping. It looks like there are two cases not covered in your test case.

jwhitmarsh · 2017-10-11T16:22:43Z

I've made the changes you suggested and updated the tests to the coverage is back up to 100% (for webpack.js). If everything is good with this, let me know if you'd like me to rebase & squash it into one commit.

lijunle

Thank you! When apply git merge, I will squash the commits. That does not matter.

lijunle · 2017-10-11T16:26:10Z

src/special/webpack.js

+function mapRuleLoaders(module) {
+  return module.rules
+    .filter(rule => rule.loaders)
+    .map(rule => rule.loaders.map(loader => loader));


Sorry, why do we still need .map(loader => loader)?

Sorry @lijunle, that was dumb of me :(!

lijunle · 2017-10-11T16:27:34Z

src/special/webpack.js

+    // filter use or loader because 'loader' is a shortcut to 'use'
+    .filter(rule => rule.use || rule.loader)
+    // return coerced array, using the relevant key
+    .map(rule => [].concat(rule[rule.use ? 'use' : 'loader']));


[].concat(rule.use || rule.loader)

lijunle · 2017-10-11T17:55:36Z

Thank you @ jwhitmarsh ! It is merged now. Do you want a new version for this?

jwhitmarsh · 2017-10-12T08:15:48Z

@lijunle It's entirely up to you - i've got a few other PR's coming your way shortly, but not sure exactly how long they will take.

lijunle · 2017-10-12T11:38:56Z

OK, I will publish it on this Saturday. If you have new ideas, please ping me in this time slot. 2017年10月12日 16:15，"James" <notifications@github.com>写道：

…

@lijunle <https://github.com/lijunle> It's entirely up to you - i've got a few other PR's coming your way shortly, but not sure exactly how long they will take. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#226 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPIdO8NSjaWZE-tgfGg9hCs0KrPZotVks5srcq0gaJpZM4Pz6vk> .

mattfysh · 2018-02-27T03:51:44Z

@lijunle was this published to npm? the version I've got from latest doesn't have the webpack2 support

feat: add webpack v2 support

87e5520

lijunle requested changes Oct 10, 2017

View reviewed changes

post PR feedback changes

ea8036c

jwhitmarsh commented Oct 10, 2017

View reviewed changes

lijunle approved these changes Oct 10, 2017

View reviewed changes

post PR feedback changes

89a0472

lijunle approved these changes Oct 11, 2017

View reviewed changes

James Whitmarsh and others added 2 commits October 11, 2017 18:01

post PR feedback changes

74198cf

Merge branch 'master' into feat/webpack-v2-support

5a8113b

lijunle merged commit f8fca26 into depcheck:master Oct 11, 2017

mattfysh mentioned this pull request Feb 28, 2018

Publish to NPM #247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add webpack v2 support #226

feat: add webpack v2 support #226

jwhitmarsh commented Oct 10, 2017

lijunle left a comment

lijunle Oct 10, 2017

lijunle Oct 10, 2017

lijunle Oct 10, 2017

lijunle Oct 10, 2017

lijunle Oct 10, 2017

lijunle Oct 10, 2017

jwhitmarsh Oct 10, 2017

jwhitmarsh Oct 10, 2017

codecov bot commented Oct 10, 2017 •

edited

jwhitmarsh Oct 10, 2017

jwhitmarsh commented Oct 10, 2017

lijunle left a comment

lijunle Oct 10, 2017

lijunle Oct 10, 2017

lijunle Oct 10, 2017

lijunle Oct 10, 2017

lijunle Oct 10, 2017

lijunle commented Oct 10, 2017

jwhitmarsh commented Oct 11, 2017

lijunle left a comment

lijunle Oct 11, 2017

jwhitmarsh Oct 11, 2017

lijunle Oct 11, 2017

lijunle commented Oct 11, 2017

jwhitmarsh commented Oct 12, 2017

lijunle commented Oct 12, 2017 via email

mattfysh commented Feb 27, 2018

feat: add webpack v2 support #226

feat: add webpack v2 support #226

Conversation

jwhitmarsh commented Oct 10, 2017

lijunle left a 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

codecov bot commented Oct 10, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

jwhitmarsh commented Oct 10, 2017

lijunle left a 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

lijunle commented Oct 10, 2017

jwhitmarsh commented Oct 11, 2017

lijunle left a 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

lijunle commented Oct 11, 2017

jwhitmarsh commented Oct 12, 2017

lijunle commented Oct 12, 2017 via email

mattfysh commented Feb 27, 2018

codecov bot commented Oct 10, 2017 •

edited