webpack-contrib / istanbul-instrumenter-loader

Istanbul Instrumenter Loader
MIT License
273 stars 65 forks source link

upgrade dependencies, including major upgrade to schema-utils #104

Closed spalger closed 3 years ago

spalger commented 3 years ago

Upgraded the dependencies to use latest versions, most were minor upgrades but schema-utils went up two major upgrades. According to the schema-utils changelog the v1 -> v2 upgrade is the only time where code changes were necessary. The changes are very minor, simply adjusting the options to pass in the name and options prefix for the validation error. Minor changes to the istanbul libraries does trigger minor snapshot upgrades but they're ver broken on github, so for the reviewers sake here are the highlights handled better by local diff highlighters:

terrible screenshot that highlights the changes in the snapshots

The upgraded package limit node version support to 8.9.0+ so I've included some pending changelog docs, upgraded the node versions being tested on CI, and upgraded to the latest version of npm to get support for node versions 12+.

Also migrated from nps check to npm audit, which should provide similar information, though I get the feeling that nobody is really looking at these results so maybe they can just be removed. There are a number of outdated packages with security vulnerabilities in the devDependencies, which should be fixed but felt out of the scope of this PR (I definitely tried to fix them) so I updated the audit command to npm audit --production to only check for security vulnerabilities in production dependencies.

Additionally, the CI checks run against webpack@latest, which require relatively minor webpack config changes to get tests passing.

jsf-clabot commented 3 years ago

CLA assistant check
All committers have signed the CLA.

spalger commented 3 years ago

Well, this has been a challenge. I'm not sure if what I've done here is too much for one PR, or if I have to get this passing against webpack canary.

I've gotten it close, but webpack canary is producing slightly different output and I'm really unsure what to do about that difference in the tests.

alexander-akait commented 3 years ago

@spalger Hi, we recently discussed to deprecate this loader, it is abandoned and not maintainable, so I did not expect a bit that someone would send PR, you can tell how you use the current loader, what is use cases?

spalger commented 3 years ago

Well, we currently use it to instrument our bundles before running them through a functional end-to-end testing suite. It's an experimental form of coverage collection and this works fine, we just had to force upgrade schema-utils with yarn resolutions and I was looking to remove a warning produced by yarn. Is there something intrinsically unmaintainable about this project that I'm not realizing? It seems like a very simple wrapper around istanbul that we could totally implement in our project rather than relying on the loader, but I'm also happy to help get this caught up to modern standards.

spalger commented 3 years ago

Also we need fix CI problems

This is tricky, as CI currently requires that webpack@latest and webpack@master both create the same bundle as verified by the Jest snapshots. There are possibly things we could do to remove problematic bits from the snapshots with serializers, or by processing the file before snapshotting it, but I think I'm trying to understand the value of running the same test suite against master and latest, especially when the difference between the two is a major version break.

Is this how things work in other webpack-contrib projects?

spalger commented 3 years ago

Oh, cool, do you have plans to fix CI? I'm happy to work up the fixes but my instinct is to just drop the webpack@master tests. Do you have a preferred solution?

alexander-akait commented 3 years ago

@spalger

Oh, cool, do you have plans to fix CI?

Yes, today

I'm happy to work up the fixes but my instinct is to just drop the webpack@master tests.

Yes, we should rewrite some tests

Do you have a preferred solution?

I think yes, let's me try