feat: Migrate to Webpack Built-in Infrastructure Logger with Backward… - #738
Conversation
🦋 Changeset detectedLatest commit: 7b4490d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
|
Thanks! Could you follow the pull request template? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #738 +/- ##
==========================================
+ Coverage 85.35% 86.75% +1.40%
==========================================
Files 17 17
Lines 1065 1110 +45
Branches 387 406 +19
==========================================
+ Hits 909 963 +54
+ Misses 142 134 -8
+ Partials 14 13 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Will do it today it self |
|
Also check the coverage report, some methods seem to bail out and not be tested |
alexander-akait
left a comment
alexander-akait
left a comment
There was a problem hiding this comment.
I like this improvement
|
Hii @valscion I have made the changes Can you review it please |
|
@valscion Any updates for me |
|
CI seems to be failing now that I triggered it to run. Is it this PR cause? |
|
@valscion It was the duplicate code. I have fixed it we are good to go |
WalkthroughThe plugin now uses Webpack’s infrastructure logger when the compiler provides it. A logger adapter preserves deprecated Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Changing the deprecated log level at runtime does not affect child loggers created afterward, so their messages may be filtered incorrectly. Fix this small compatibility gap before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 7 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
src/BundleAnalyzerPlugin.jsESLint failed to execute (timeout). src/Logger.jsESLint skipped: the matched ESLint configuration already failed (timeout). src/analyzer.jsESLint skipped: the matched ESLint configuration already failed (timeout).
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: b4233e54-d4e7-479d-90e8-bae43eaf8463
📒 Files selected for processing (9)
.changeset/use-webpack-infrastructure-logger.mdREADME.mdsrc/BundleAnalyzerPlugin.jssrc/Logger.jssrc/analyzer.jssrc/utils.jssrc/viewer.jstest/Logger.jstest/plugin.js
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| return (/** @type {string | (() => string)} */ name) => | ||
| Logger.createInfrastructureLoggerAdapter( | ||
| target.getChildLogger(name), | ||
| userLogLevel, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the current log level for new child loggers.
setLogLevel updates activeLevels, but getChildLogger passes the original userLogLevel. For example, after changing "error" to "warn", a new child logger still suppresses warnings.
Track the current level and pass it to createInfrastructureLoggerAdapter. Add a test that creates a child after setLogLevel.
Proposed fix
- const levelIndex = LEVELS.indexOf(userLogLevel);
+ let currentLevel = userLogLevel;
+ const levelIndex = LEVELS.indexOf(currentLevel);
if (prop === "setLogLevel") {
return (/** `@type` {Level} */ level) => {
const idx = LEVELS.indexOf(level);
if (idx === -1) {
throw new Error(
`Invalid log level "${level}". Use one of these: ${LEVELS.join(", ")}`,
);
}
+ currentLevel = level;
activeLevels.clear();
for (const [i, l] of LEVELS.entries()) {
if (i >= idx) activeLevels.add(l);
}
};
}
Logger.createInfrastructureLoggerAdapter(
target.getChildLogger(name),
- userLogLevel,
+ currentLevel,
true,
);There was a problem hiding this comment.
This doesn't look like a code flow which users should be using. So this is a false positive.
valscion
left a comment
valscion
left a comment
There was a problem hiding this comment.
Yeah this looks good to me! Thanks!
3 participants
This pull request updates the logging system of the
webpack-bundle-analyzerplugin to integrate with Webpack's native infrastructure logger when available. It introduces a new adapter for compatibility, deprecates the plugin'slogLeveloption in favor of Webpack'sinfrastructureLogging, and updates documentation and type annotations accordingly.Logging integration and deprecation:
compiler.getInfrastructureLogger('webpack-bundle-analyzer')when available, providing better integration with Webpack's logging system.logLeveloption is deprecated in favor of Webpack'sinfrastructureLogging.level, with documentation and type comments updated to reflect this change. A deprecation warning is shown iflogLevelis used. [1] [2] [3] [4] [5]Logger implementation and type updates:
InfrastructureLoggerAdapterinLogger.jsto bridge between the plugin's logger interface and Webpack's infrastructure logger, supporting log level filtering and deprecation warnings. [1] [2] [3]src/analyzer.js,src/utils.js,src/viewer.js) to accept either the customLoggeror Webpack's infrastructure logger for improved type safety and compatibility. [1] [2] [3] [4] [5] [6] [7]These changes ensure that logging is consistent with Webpack's ecosystem and prepare for the eventual removal of the plugin's custom
logLeveloption compatibility#358
@valscion if you can review it
Summary by CodeRabbit
logLeveloption is deprecated. Configure logging levels through Webpack’s nativeinfrastructureLoggingsettings instead.logLeveldeprecation.