Skip to content

Replace Winston with in-house logger implementation - #543

Open
Jayden Chan (jayden-chan) wants to merge 52 commits into
masterfrom
jayden/remove-winston
Open

Jayden Chan (jayden-chan) wants to merge 52 commits into
masterfrom
jayden/remove-winston

Conversation

@jayden-chan

@jayden-chan Jayden Chan (jayden-chan) commented Sep 15, 2026

Copy link
Copy Markdown
Member

Replaces Winston with a much more lightweight in-house logger implementation.

Rationale:

  • Improve performance by removing unnecessary features and formatting machinery
  • Reduce long term maintenance burden by removing Winston dependency
  • Reduce attack surface by removing Winston dependency
  • Add dedicated log collection mechanism which fixes a bug with the old Winston-based logger

Copilot AI balanced review requested due to automatic review settings September 15, 2026 21:02

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings September 15, 2026 21:12

This comment was marked as resolved.

This commit was generated by GitHub Actions CI
Copilot AI review requested due to automatic review settings September 15, 2026 21:19

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings September 15, 2026 21:41

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings September 15, 2026 21:55

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings September 15, 2026 22:03
This commit was generated by GitHub Actions CI

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings September 15, 2026 22:06

This comment was marked as duplicate.

Copilot AI review requested due to automatic review settings September 18, 2026 18:17

This comment was marked as low quality.

Copilot AI review requested due to automatic review settings September 18, 2026 18:25

This comment was marked as resolved.

This is quite a bit of code to handle what will most likely be an
extremely rare edge case, but I'd rather deal with this before it
becomes a problem in production and causes a real headache
Copilot AI review requested due to automatic review settings September 18, 2026 21:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Shared-stream backpressure, stale logger configuration, and release compatibility remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • packages/logger/package-lock.json: Generated file

Suppressed comments (2)

packages/logger/README.md:49

  • The public Sink API has no type discriminator or separate console variant; every sink is configured with a Node.js Writable stream. This sentence advertises sink types that do not exist. Describe stdout and stderr as writable-stream examples instead.
The supported sink types are `console` and `stream`.

packages/logger/CHANGELOG.md:19

  • This release note still says callers can configure a console sink, but the replacement API exposes only writable-stream sinks. Update the migration guidance so it matches the new Sink type.
- Removed `SSQT_LOG_TRANSPORTS`; configure `console` or writable `stream` sink through `initializeLogger`.
  • Files reviewed: 32/34 changed files
  • Comments generated: 4
  • Review effort level: Balanced

"@eslint/eslintrc": "^3.3.5",
"@eslint/js": "^10.0.1",
"@saasquatch/logger": "^3.0.0",
"@saasquatch/logger": "^3.0.1-7",
Comment thread packages/logger/src/stream-sink.ts Outdated
Comment thread packages/express-boilerplate/package.json Outdated
Comment thread packages/express-boilerplate/src/shutdown.ts
Not worth implementing considering Winston doesn't handle it correctly
and we haven't experienced any issues in 5+ years of using it in
production.
Copilot AI review requested due to automatic review settings September 18, 2026 21:54

This comment was marked as duplicate.

Copilot AI review requested due to automatic review settings September 18, 2026 22:21

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings September 18, 2026 22:55
github-actions Bot and others added 2 commits September 18, 2026 22:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

An Express test helper still uses the removed logLevel option, causing compilation failure or incorrect logger behavior.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (3)
Files not reviewed (1)
  • packages/logger/package-lock.json: Generated file
Previously missed (3)

In code that hasn't changed since last review

Medium severity Update test logger configuration and sinks

packages/​logger/​src/​logger.ts:57

LoggerConfig no longer accepts logLevel, but packages/express-boilerplate/src/tests/util.ts:12-14 still initializes the test logger with { logLevel: "crit" }. That excess property fails TypeScript compilation now that Express resolves this API; if type checking is bypassed, it is silently ignored and the logger uses the default info level and stdout sink. Update the helper to use level: "crit" and explicitly set sinks if tests should remain quiet.

Low severity Correct migration note for writable-stream sink configuration

packages/​logger/​CHANGELOG.md:19

The replacement API has no console or stream sink discriminator; each sink is { stream: Writable }. Update this migration note so it describes the actual initializeLogger configuration.

Low severity Document sinks as writable streams only

packages/​logger/​README.md:49

The public Sink API only accepts { stream: Writable }; it has no console or stream type discriminator. This sentence advertises configuration variants that consumers cannot use. Describe writable-stream sinks instead.

Copilot AI review requested due to automatic review settings September 18, 2026 23:01
This commit was generated by GitHub Actions CI

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

Remaining feedback is limited to minor documentation consistency issues.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Files not reviewed (1)
  • packages/logger/package-lock.json: Generated file
Previously missed (3)

In code that hasn't changed since last review

Low severity Add missing 4.0.0 changelog link reference

packages/​logger/​CHANGELOG.md:10

This reference-style [4.0.0] heading has no matching link definition at the bottom of the changelog, unlike the existing release headings, so it renders as plain bracketed text. Add the 4.0.0 release reference when the release URL is finalized.

Low severity Document supported writable-stream Sink configuration

packages/​logger/​CHANGELOG.md:19

The new Sink API accepts only { stream: Writable }; there is no console sink kind. This migration note should describe the supported writable-stream configuration rather than direct users to a removed transport type.

Low severity Correct Sink documentation to describe writable streams

packages/​logger/​README.md:49

The exported Sink type is { stream: Writable }; it has no console or stream discriminator. This sentence could lead users to try the removed transport-style configuration, so describe stdout and stderr as writable streams instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants