Upgrade winston dependency to v3#2858
Conversation
Winston is on v2. v2 doesn't have proper ESM support, resulting in a workaround being required to import it. Upgrade winston package dependency so that normal `import` syntax may be used.
DailyRotateFile's updated version now outputs an audit file to track files. This interferes with the CLI log-cleaning function on a failed build. Explicitly specify an audit file output directory that is in line with the other log directories. Update cleaning function to remove this on failed build.
Core winston uses v2 CLI logging uses the fact that the shared winston object can be configured to add on trasports (like file logging). This means that if different winston versions are used across core and cli, the shared winston object becomes individual to each package, ultimately resulting in the logging not working properly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2858 +/- ##
==========================================
+ Coverage 71.89% 71.90% +0.01%
==========================================
Files 132 132
Lines 7359 7362 +3
Branches 1542 1610 +68
==========================================
+ Hits 5291 5294 +3
- Misses 1967 2062 +95
+ Partials 101 6 -95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates MarkBind’s logging stack to Winston v3 (and winston-daily-rotate-file v5) across @markbind/core and markbind-cli, removing the prior CommonJS compatibility wrapper and adjusting CLI cleanup to account for the new rotate-file audit file.
Changes:
- Bump
winstonfrom^2.4.4to^3.19.0in both core and CLI; bumpwinston-daily-rotate-fileto^5.0.0in CLI. - Replace Winston v2 transport options with Winston v3
formatpipelines in core/CLI loggers and remove thelogger-wrapper.cjsshim. - Update CLI cleanup to delete the rotate-file
audit.jsonbefore attempting to remove_markbind/logs.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/utils/logger.ts | Switch core console transport to Winston v3 formatting APIs. |
| packages/core/package.json | Upgrade core dependency to Winston v3. |
| packages/cli/src/util/logger.ts | Remove wrapper usage; configure Winston v3 formats and DailyRotateFile v5 usage. |
| packages/cli/src/util/logger-wrapper.cjs | Deleted legacy Winston v2 CommonJS wrapper. |
| packages/cli/src/util/cliUtil.ts | Add audit file deletion so _markbind/logs can be removed when otherwise empty. |
| packages/cli/package.json | Upgrade CLI dependencies to Winston v3 and winston-daily-rotate-file v5. |
| package-lock.json | Lockfile updates reflecting the dependency upgrades. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Logging Level Configuration
Because of the hoisting of single copy of winston to top level node_modules, both core and cli which depend on same ver of winston will resolve to same instance, hence the core logic will respect the consoleTransport.level set in CLI.
- CLI imports Core first
$\rightarrow$ winston is by default set todebug. - CLI imports its own logger second
$\rightarrow$ winston is overwritten toinfo, todebugfor dev mode.
Slightly hacky as it relies on side effects of import order and global state, but this can be an issue for another day.
- Which is why it is important to have both versions to be same and not drift.
Debug logs showing up
The reason behind the debug logs appearing in #2850 was possibly due to some dependency update done in migration efforts, but upgrading to winston v3, due to change in how winston handles configuration, fixes it.
Debug logs now show up in the temporary built log files instead of the CLI!
- Didn't notice any changes to the log formats.
- Further observation of whether the issue reappears or otherwise would be good in the near term, otherwise we can consider it as fixed.
Let me know if I misunderstood anything! Just appended insights and understanding on the issue for future reference if needed. Great work done in the PR.
LGTM
Thanks for the PR @Harjun751
What is the purpose of this pull request?
Resolves #2855
Resolves #2850
Overview of changes:
Upgrades both CLI and Core to Winston v3.
Anything you'd like to highlight/discuss:
I had to upgrade the Core winston due to reasons highlighted in the commit message for cd4012a. Might be worth finding a way to peg the versions together so they never diverge. I tried utilizing npm workspaces to achieve this, but I couldn't get it to work (anyone knows why ?? :*()
Also, fixing the debug error was a happy little accident as it fixed itself once I configured v3.
Testing instructions:
Check that the log file format is similar to your older logs, if possible. They should be OK.
Proposed commit message: (wrap lines at 72 characters)
Upgrade winston dependency to v3
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):