webpack-contrib / istanbul-instrumenter-loader

Istanbul Instrumenter Loader

MIT License

273 stars 65 forks source link

Update istanbul-lib-instrument from ^1.7.3 to ^1.9.2 #99

Closed abdulrahman-khankan closed 4 years ago

abdulrahman-khankan commented 4 years ago

Update the istanbul-lib-instrument version to get /* istanbul ignore file */. Reference to issue
Update the CI pipelines to use Webpack version 3.8.1 which is the what is specified in the package.json file. Leaving it as latest will break the builds since v4 requires migrating the configuration and possibly more code.

codecov[bot] commented 4 years ago

Codecov Report

Merging #99 into master will not change coverage. The diff coverage is n/a.

@@          Coverage Diff           @@
##           master     #99   +/-   ##
======================================
  Coverage    92.3%   92.3%           
======================================
  Files           2       2           
  Lines          13      13           
  Branches        2       2           
======================================
  Hits           12      12           
  Misses          1       1

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 fe5fedb...19cc0eb. Read the comment docs.

abdulrahman-khankan commented 4 years ago

@evilebottnawi @michael-ciniawsky @deepsweet @bebraw @mattlewis92 could any of you guys please approve this? 🙂

abdulrahman-khankan commented 4 years ago

@evilebottnawi after upgrading to latest packages + Webpack 4, I cannot get the snapshots to match anymore, it is different between my environments (Windows & Linux) and Travis, and Appveyor Before the upgrade my snapshots generated on Linux were matching Travis & Appveyor, but not anymore.

Travis:

Appveyor:

Linux is what is committed right now, and generating on Windows is producing even another version.

Can I just revert that upgrade and leave it for another PR?

I agree that it is bad to lock the version of Webpack to 3.8 in the CI pipelines, but isn't the improvement that my PR brings better than no improvement at all?

With the current status, no one can make any PR at all without upgrading to Webpack 4, and the maintainers don't have time to to upgrade it. So by not allowing improvements that require locking the Webpack version, it is killing any chance to improve the project and keep it alive.

abdulrahman-khankan commented 4 years ago

I have reverted my changes of the babel & webpack v4 upgrade as it will take me some time to update the tests and migrate latest webpack-defaults as well.

Please read my comment above and approve this for now, and I will work on the upgrade separately. Please note that the hardcoded values in travis/appveyor will not matter because the new webpack-defautls uses azure as a CI, so both .travis and .appveyor files will be deleted during the upgrade.

abdulrahman-khankan commented 4 years ago

@evilebottnawi I did update all possible packages. Webpack and Babel upgrades are major v3 -> v4 and v6 -> v7 respectively. With the currently used ^version this PR has the latest possible dependencies.

In package.json we have

"babel-cli": "^6.26.0",
"webpack": "^3.8.1"

Those two packages only have major upgrades and will not be covered by the ^

alexander-akait commented 4 years ago

@abdulrahman-khankan I understand you, but this PR really doesn’t improve anything, just raising the versions that are already the to be last during installation, we have to do something more in PRs, I would like to see drop webpack@3 here, anyway thanks for the PR

abdulrahman-khankan commented 4 years ago

@evilebottnawi I see your point now. I will make a new PR with the upgrades when it is ready.