Experiment with adding webpack 4 support back

[Copilot]

• Identify actionable new @copilot comments
• Review the full PR diff for correctness, compatibility, and risk
• Summarize key strengths, concerns, and follow-up recommendations
• Reply to the new PR comment with concise review thoughts

[johnnyreilly]

@copilot please consistently use instance.isWebpack5 to decide strategy

[Copilot]

@copilot please consistently use instance.isWebpack5 to decide strategy

Done in 5d7ab2f. I updated the remaining strategy checks to consistently branch on instance.isWebpack5 (including cached/reused instance paths and build-dependency registration) rather than using direct feature detection at those decision points.

[johnnyreilly]

Is this necessary @copilot?

[Copilot]

Good catch — no, it wasn’t necessary there. I removed isWebpack5 from utils.ts and localized detection to instance setup in instances.ts in 78fa91f, so strategy still flows through instance.isWebpack5 without the extra utils export.

[johnnyreilly]

@copilot I don't think the webpack5 code needs to use webpackModule it can use module

[Copilot]

Updated in 7b3cd3e — webpack 5 path now uses module directly (getWarnings/getErrors/clearWarningsAndErrors) and keeps webpackModule only for the webpack 4 branch.

[Copilot]

@copilot please consistently use instance.isWebpack5 to decide strategy

Handled in 5d7ab2f and retained in follow-ups: strategy decisions branch on instance.isWebpack5 consistently.

[Copilot]

@copilot after the last commit all yarn comparison-tests are failing

Fixed in 90b2860. I checked the failing CI jobs and they were failing before test execution with TS2416 type errors from @types/eslint-scope after the last lockfile change. I restored the prior lockfile resolution (the last commit had collapsed @types/eslint@*), which unblocked comparison test compilation. I then re-ran yarn build, yarn lint, yarn comparison-tests -- --single-test errors, and yarn comparison-tests -- --single-test nodeModulesMeaningfulErrorWhenImportingTs successfully.

[johnnyreilly]

Starting to write this up here: johnnyreilly/blog.johnnyreilly.com#1109

[johnnyreilly]

@copilot review the src/**.*ts files in this PR with an eye to identifying any potential performance issues this PR could introduce. There may be none. Tell me what issues might exist. Make no changes - let me decide.

[Copilot]

@copilot review the src/**.*ts files in this PR with an eye to identifying any potential performance issues this PR could introduce. There may be none. Tell me what issues might exist. Make no ch...

Reviewed at 06def35 with a performance-focused pass over src/**/*.ts changes in this PR. I don’t see any high-confidence new perf regressions.

Possible low-risk hotspots to keep an eye on:

• src/after-compile.ts:131-146 (determineModules): now also checks resourceResolveData?.path; this may increase modules considered in the map in some builds.
• src/watch-run.ts:42-49 (timestamp normalization): extra per-file branching on each watch cycle; likely negligible, but it is new work on very large file sets.

Everything else I reviewed looks neutral or slightly better for runtime cost (no obvious new heavy loops/allocations in hot paths). No code changes made.

[johnnyreilly]

@copilot please review this PR and give me your thoughts

[Copilot]

@copilot please review this PR and give me your thoughts

My review on 8332763:

Overall this is in good shape and I think the webpack 4 support direction is sound. The latest branch CI I checked for this head is green (build and test completed successfully).

What looks strong:

• Optional loader-utils peer dependency with webpack-4-only runtime loading is a good compatibility strategy.
• The webpack 4 module error normalization path in after-compile now behaves consistently with restored comparison snapshots.
• The watch timestamp handling changes look robust across webpack timestamp shapes.

Things I’d keep an eye on before merge:

• Version strategy is now partly driven by global webpack.version (src/loaderUtils.ts) rather than only a passed-through per-instance flag; that’s workable but a potential future drift point.
• determineModules now also uses resourceResolveData?.path (src/after-compile.ts), which can increase module map population in large compilations (likely low risk, but worth noting).
• yarn.lock churn is large; if possible, keep it as tight as practical.

I don’t see a clear blocker in the current state.