webpack-contrib / istanbul-instrumenter-loader

Istanbul Instrumenter Loader
MIT License
273 stars 65 forks source link

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

Closed mattlewis92 closed 6 years ago

mattlewis92 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@hulkish done!

michael-ciniawsky commented 6 years ago

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

mattlewis92 commented 6 years ago

@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 commented 6 years ago

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) ? 🙃

mattlewis92 commented 6 years ago

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 commented 6 years ago

Drop the nodemon dependency please and g2g :)

mattlewis92 commented 6 years ago

Done! 💃

mattlewis92 commented 6 years ago

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 commented 6 years ago

@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.

joshwiens commented 6 years ago

Done - 3.0.0-beta.1

mattlewis92 commented 6 years ago

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 😀