refactor: simplify source code && update `webpack-defaults v1.1.0...1.5.0` #63

mattlewis92 · 2017-07-09T15:42:14Z

It looks like travis / appveyor aren't enabled on this repo yet so the tests won't run, @d3viant0ne would you mind enabling these when you have a sec please?

I think this should be enough for an RC release. Any feedback is welcome!

p.s. I removed all the sourcemap stuff from the codebase as it looks like the loader API in webpack 2+ changed so you can't get the source map from the previous loader and webpack will auto merge sourcemaps for you. Previously sourcemaps were always generated by default due to a bug in the source so I've left that on by default to preserve backwards compatibility (99% of the time users are going to want to produce sourcemaps, otherwise it will be impossible to debug their code)

hulkish · 2017-07-09T16:05:39Z

@mattlewis92 Can you also include compilation warnings/errors in your snapshot tests? example: https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/master/test/all-options.test.js#L70

mattlewis92 · 2017-07-09T16:13:26Z

@hulkish done!

michael-ciniawsky · 2017-07-09T17:23:15Z

src/index.js

-    srcMap = inlineSourceMap.sourcemap;
-  }
+export default function (source) {
+  const userOptions = loaderUtils.getOptions(this) || {};


userOptions => options

michael-ciniawsky · 2017-07-09T17:25:27Z

test/helpers.js

+import MemoryFileSystem from 'memory-fs';
+import webpack from 'webpack';
+
+const istanbulInstrumenterLoader = require.resolve('../src/cjs.js');


- const istanbulInstrumenterLoader + const loader

michael-ciniawsky · 2017-07-09T17:31:21Z

test/helpers.js

+      path: path.join(__dirname, 'fixtures', 'dist'),
+    },
+    module: {
+      rules: [{


[ { test: /\.js$/ - loader: istanbulInstrumenterLoader, + loader, - options: instrumenterOptions, + options, enforce: 'post' } ]

michael-ciniawsky · 2017-07-09T17:32:58Z

test/helpers.js

@@ -0,0 +1,40 @@
+import path from 'path';


Rename to (webpack|compiler|run).js

michael-ciniawsky · 2017-07-09T17:34:06Z

test/helpers.js

+  });
+}
+
+export function stripLocalPaths(source) {


Move to test/helpers.js (Separate file to compiler)

michael-ciniawsky · 2017-07-09T17:35:12Z

test/helpers.js

+}
+
+export function stripLocalPaths(source) {
+  const regexpReplace = new RegExp(__dirname, 'g');


regexReplace => replacer || replacee || regex

michael-ciniawsky · 2017-07-09T17:36:26Z

test/helpers.js

+  }, extra);
+}
+
+export function runWebpack(config) {


Make this the default export without a name, see below why :)

michael-ciniawsky · 2017-07-09T17:38:15Z

test/index.test.js

@@ -0,0 +1,34 @@
+import { runWebpack, webpackConfigFactory, stripLocalPaths } from './helpers';


import webpack from './webpack'; // Constistent across the webpack-contrib (`webpack-defaults`) import { stripLocalPaths } from './helpers'; // Custom helpers per repo

michael-ciniawsky · 2017-07-09T17:50:34Z

test/helpers.js

+
+const istanbulInstrumenterLoader = require.resolve('../src/cjs.js');
+
+export function webpackConfigFactory({ instrumenterOptions, extra, fixtureEntry = 'basic.js' } = {}) {


Move into runWebpack

michael-ciniawsky · 2017-07-09T17:53:09Z

test/helpers.js

+  }, extra);
+}
+
+export function runWebpack(config) {


export default function (fixture, options, extend = {}) { const config = Object.assign({ ...webpackConfigFactory }, extend) ...runWebpack }

michael-ciniawsky · 2017-07-09T17:58:47Z

Please also split chore, test, refactor tasks into micro PRs accordingly whenever possible, I ensure you it will get reviewed :)

mattlewis92 · 2017-07-09T18:02:28Z

@michael-ciniawsky it was kind of tricky to do that, since each subsequent commit depended on changes from the previous commit. It's probably best to just squash and merge these changes into one commit anyway 😄

michael-ciniawsky · 2017-07-09T18:31:54Z

Mind to open a PR in webpack-defaults with your initial test setup test/(compiler|run|webpack).js to start working on #68 (webpack-defaults) ? 🙃

michael-ciniawsky · 2017-07-09T19:14:19Z

package.json

@@ -48,7 +48,6 @@
    "eslint-config-webpack": "^1.2.5",
    "eslint-plugin-import": "^2.7.0",
    "jest": "^20.0.4",
-    "jest-cli": "^20.0.4",
    "lint-staged": "^3.6.1",
    "memory-fs": "^0.4.1",
    "nodemon": "^1.11.0",


Not needed anymore since ~webpack-defaults v1.3.0+

mattlewis92 · 2017-07-09T19:51:46Z

Had a minor heart attack for one sec as I messed up the rebase then force pushed the changes and lost all my work... thankfully webstorm has a file restore feature 😅

@michael-ciniawsky I've addressed your review changes and squashed the commits into 2 - one for upgrading webpack-defaults and the other for making the produceSourceMap on by default to maintain BC with 2.x + added tests to verify the behaviour. LMK if you'd like me to change anything else 😄

michael-ciniawsky

👍

michael-ciniawsky · 2017-07-09T20:53:58Z

package.json

    "jest": "^20.0.4",
-    "lint-staged": "^3.4.0",
+    "lint-staged": "^3.6.1",
+    "memory-fs": "^0.4.1",
    "nodemon": "^1.11.0",


michael-ciniawsky · 2017-07-09T21:10:44Z

Drop the nodemon dependency please and g2g :)

mattlewis92 · 2017-07-09T21:21:21Z

Done! 💃

mattlewis92 · 2017-07-09T21:29:59Z

Awesome thanks @michael-ciniawsky ! Would it be possible for either @d3viant0ne or @bebraw to cut a new beta with this change please? Thanks! 😄

btw there was actually a fix in with this PR, previously produceSourceMap was off and now it's on by default. It's probably worth adding this to the changelog for the release when its generated (although the previous release was broken so I doubt anyone would have had issues with it)

joshwiens · 2017-07-09T22:06:54Z

@mattlewis92 - Not sure why Travis & Appveyor weren't enabled but that test suite fails locally.

The lib works, so i'll push a new beta & we can deal with the snapshots later.

- `produceSourceMap` is now on by default & must be explicitly disabled if so desired

joshwiens · 2017-07-10T01:06:42Z

Done - 3.0.0-beta.1

mattlewis92 · 2017-07-10T06:37:59Z

Ah it's because the webpack build hashes are different, they're non-deterministic by default and not based off the file contents. I will just strip them from the snapshots the same way I do for the file paths, will send a PR later 😀

mattlewis92 force-pushed the tests branch from 771c154 to 7e686b9 Compare July 9, 2017 15:42

joshwiens added priority: 3 (required) pr_action: Review semver: Major type: Refactor labels Jul 9, 2017

joshwiens requested review from hulkish, alexander-akait and joshwiens July 9, 2017 15:49

joshwiens changed the title ~~Update webpack defaults, add tests, simplify source code~~ refactor: Update webpack defaults, add tests, simplify source code Jul 9, 2017

hulkish approved these changes Jul 9, 2017

View reviewed changes

joshwiens approved these changes Jul 9, 2017

View reviewed changes

joshwiens added semver: Patch and removed semver: Major labels Jul 9, 2017

michael-ciniawsky suggested changes Jul 9, 2017

View reviewed changes

michael-ciniawsky added this to the 3.0.1 milestone Jul 9, 2017

michael-ciniawsky self-assigned this Jul 9, 2017

michael-ciniawsky changed the title ~~refactor: Update webpack defaults, add tests, simplify source code~~ refactor: simplify source code && update webpack-defaults v1.1.0...1.5.0 Jul 9, 2017

michael-ciniawsky reviewed Jul 9, 2017

View reviewed changes

chore: upgrade webpack-defaults

8c8f446

mattlewis92 force-pushed the tests branch from fd83ef8 to 8c8f446 Compare July 9, 2017 19:35

fix: produceSourceMap should be true by default

11cfe10

michael-ciniawsky mentioned this pull request Jul 9, 2017

feat: add options validation (schema-utils) #65

Merged

michael-ciniawsky approved these changes Jul 9, 2017

View reviewed changes

chore: remove nodemon dev dependency

67e1d8c

michael-ciniawsky merged commit f6a7c2d into webpack-contrib:master Jul 9, 2017

michael-ciniawsky added status: Approved and removed priority: 3 (required) pr_action: Review labels Jul 9, 2017

mattlewis92 deleted the tests branch July 9, 2017 21:24

joshwiens pushed a commit that referenced this pull request Jul 10, 2017

refactor: simplify source code && update defaults (#63)

182ce6a

- `produceSourceMap` is now on by default & must be explicitly disabled if so desired

michael-ciniawsky removed this from the 3.0.2 milestone Oct 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: simplify source code && update `webpack-defaults v1.1.0...1.5.0` #63

refactor: simplify source code && update `webpack-defaults v1.1.0...1.5.0` #63

mattlewis92 commented Jul 9, 2017

hulkish commented Jul 9, 2017

mattlewis92 commented Jul 9, 2017

michael-ciniawsky Jul 9, 2017

michael-ciniawsky Jul 9, 2017

michael-ciniawsky Jul 9, 2017

michael-ciniawsky Jul 9, 2017

michael-ciniawsky Jul 9, 2017

michael-ciniawsky Jul 9, 2017

michael-ciniawsky Jul 9, 2017

michael-ciniawsky Jul 9, 2017 •

edited

michael-ciniawsky Jul 9, 2017

michael-ciniawsky Jul 9, 2017

michael-ciniawsky commented Jul 9, 2017

mattlewis92 commented Jul 9, 2017

michael-ciniawsky commented Jul 9, 2017 •

edited

michael-ciniawsky Jul 9, 2017 •

edited

mattlewis92 commented Jul 9, 2017

michael-ciniawsky left a comment

michael-ciniawsky Jul 9, 2017

michael-ciniawsky commented Jul 9, 2017

mattlewis92 commented Jul 9, 2017

mattlewis92 commented Jul 9, 2017

joshwiens commented Jul 9, 2017 •

edited

joshwiens commented Jul 10, 2017

mattlewis92 commented Jul 10, 2017

		@@ -0,0 +1,34 @@
		import { runWebpack, webpackConfigFactory, stripLocalPaths } from './helpers';


		const istanbulInstrumenterLoader = require.resolve('../src/cjs.js');

		export function webpackConfigFactory({ instrumenterOptions, extra, fixtureEntry = 'basic.js' } = {}) {

refactor: simplify source code && update webpack-defaults v1.1.0...1.5.0 #63

refactor: simplify source code && update webpack-defaults v1.1.0...1.5.0 #63

Conversation

mattlewis92 commented Jul 9, 2017

hulkish commented Jul 9, 2017

mattlewis92 commented Jul 9, 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

michael-ciniawsky Jul 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky commented Jul 9, 2017

mattlewis92 commented Jul 9, 2017

michael-ciniawsky commented Jul 9, 2017 • edited

michael-ciniawsky Jul 9, 2017 • edited

Choose a reason for hiding this comment

mattlewis92 commented Jul 9, 2017

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky commented Jul 9, 2017

mattlewis92 commented Jul 9, 2017

mattlewis92 commented Jul 9, 2017

joshwiens commented Jul 9, 2017 • edited

joshwiens commented Jul 10, 2017

mattlewis92 commented Jul 10, 2017

refactor: simplify source code && update `webpack-defaults v1.1.0...1.5.0` #63

refactor: simplify source code && update `webpack-defaults v1.1.0...1.5.0` #63

michael-ciniawsky Jul 9, 2017 •

edited

michael-ciniawsky commented Jul 9, 2017 •

edited

michael-ciniawsky Jul 9, 2017 •

edited

joshwiens commented Jul 9, 2017 •

edited