Skip to content

chore(logger): bumped consola - #241

Draft
YuryShkoda wants to merge 1 commit into
mainfrom
cli-issue-1367
Draft

chore(logger): bumped consola#241
YuryShkoda wants to merge 1 commit into
mainfrom
cli-issue-1367

Conversation

@YuryShkoda

Copy link
Copy Markdown
Member

Issue

Related issue sasjs/cli#1367

Intent

  • Logger should be able to disable colors in logged messages.

Implementation

  • Bumped version of consola.

Checks

  • Code is formatted correctly (npm run lint:fix).
  • All unit tests are passing (npm test).
  • All sasjs-cli unit tests are passing (npm test).
  • Reviewer is assigned.

Reviewer checks

  • Any new code is documented.

@4gl-reviewer 4gl-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hermes Agent Code Review

Verdict: REQUEST_CHANGES

Summary

This PR bumps consola from 2.15.0 to 3.2.3 (a major version upgrade), and also upgrades typescript from 4.5.4 to 5.1.6 and @types/node from 17.0.12 to 20.4.9. The consola import style is updated from a default import to a named import (import { consola } from 'consola'), which is the correct approach for consola v3.

Pre-review Checks

  • Mergeable: false (mergeable_state: dirty) — this PR has merge conflicts with main and must be rebased before merging.
  • CI: No check runs found for the head SHA.

Issues Found

Correctness — TypeScript 5 upgrade breaks a test suite (blocking)

The typescript 4.5 to 5.1 and @types/node 17 to 20 upgrades cause a TypeScript compilation error in src/file/spec/file.spec.ts. The unifyFilePath function defaults its separator parameter to path.sep, which is now typed as '\\' | '/' instead of string. The test passes '$', producing:

error TS2345: Argument of type '"$"' is not assignable to parameter of type '"\\" | "/" | undefined'.

Fix: Explicitly type the separator parameter as string in src/file/file.ts:166.

Tests — Import style inconsistency

src/logger/logger.ts uses import { consola } from 'consola' (named import), but src/logger/Logger.spec.ts still uses import consola from 'consola' (default import). Tests currently pass, but this inconsistency should be resolved for robustness.

Style — Leftover TODO comment

src/logger/isLowerThanOrEqualTo.ts:7 contains // REFACTOR: use switch — a development note that should either be acted upon or removed before merge.

Scope — PR title understates the changes

The title mentions only consola, but typescript and @types/node are also bumped with significant implications.

Pre-existing failures

src/sasjsCli/specs/getInitTerm.spec.ts has 12 failing tests due to ENOENT errors — these are pre-existing and unrelated to this PR (confirmed by running against the base branch).

Verification Performed

  • Cloned repo, checked out PR branch, ran npm install
  • Ran npx tsc --noEmit — passes
  • Ran full test suite (npx jest) — 370 passed, 12 failed (pre-existing), 1 suite failed to compile (file.spec.ts)
  • Confirmed consola v3 named import works correctly at runtime
  • Checked npm audit — no vulnerabilities introduced by consola 3.2.3

Verdict

Request changes. The TypeScript 5 upgrade causes a compile error in src/file/spec/file.spec.ts that must be fixed before merge. Additionally, merge conflicts need to be resolved, and the import style inconsistency in the test file should be addressed.

Reviewed by Hermes Agent (GitHub App)

Comment thread src/logger/logger.ts
@@ -1,4 +1,4 @@
import consola from 'consola'
import { consola } from 'consola'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tests: The source file now uses the named import import { consola } from 'consola', but the test file src/logger/Logger.spec.ts still uses the default import import consola from 'consola'. While the tests currently pass (because in consola v3's CJS build consola.consola === consola), this inconsistency is fragile. If the module structure changes or the mock behavior diverges, the spy targets in the tests could stop matching the calls in the source. Recommend updating the test file to use the same named import style for consistency and robustness.

currentLevel: LogLevel,
level: LogLevel
): boolean => {
// REFACTOR: use switch

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Style: The // REFACTOR: use switch comment looks like a development note/TODO. Either implement the switch refactor in this PR or remove the comment before merging, since leaving TODO-style comments without an associated issue reduces code clarity.

Comment thread package.json
"ts-loader": "9.2.6",
"ts-node": "10.9.1",
"typescript": "4.5.4"
"typescript": "5.1.6"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Correctness: Bumping typescript from 4.5.4 to 5.1.6 is a major version jump that introduces stricter type checking. This causes a compile error in src/file/spec/file.spec.ts:184:

error TS2345: Argument of type '"$"' is not assignable to parameter of type '"\\" | "/" | undefined'.

This happens because @types/node@20 types path.sep as '\\' | '/' (a literal union) rather than string as in @types/node@17. The unifyFilePath function in src/file/file.ts:166 defaults its separator parameter to path.sep, inheriting the narrow type. The test passes '$' which is no longer valid.

Fix: Widen the parameter type in unifyFilePath:

export function unifyFilePath(
  filePath: string,
  separator: string = path.sep,
  separatorToReplace = '/'
)

Note: ts-jest also warns that TypeScript 5.1.6 has not been tested with ts-jest 29.0.5 (supported range is >=4.3.0 <5.0.0).

Comment thread package.json
"@types/cli-table": "0.3.0",
"@types/find": "0.2.1",
"@types/jest": "29.4.0",
"@types/node": "20.4.9",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Scope: The PR title says "chore(logger): bumped consola", but this commit also bumps typescript (4.5.4 to 5.1.6) and @types/node (17.0.12 to 20.4.9). These are significant upgrades that warrant mention in the PR title or description, since they introduce breaking type changes (see comment on line 52).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant