Skip to content

feat(client-core): Improve format API#10692

Merged
ovr merged 4 commits intomasterfrom
fix/client-core-format
Apr 15, 2026
Merged

feat(client-core): Improve format API#10692
ovr merged 4 commits intomasterfrom
fix/client-core-format

Conversation

@ovr
Copy link
Copy Markdown
Member

@ovr ovr commented Apr 15, 2026

No description provided.

@ovr ovr requested a review from a team as a code owner April 15, 2026 13:09
@github-actions github-actions bot added client:core Issues relating to the JavaScript client SDK javascript Pull requests that update Javascript code labels Apr 15, 2026
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 15, 2026

Claude finished @ovr's task in 2m 52s —— View job


Code Review: feat(client-core): Improve format API

  • Gather context and read changed files
  • Review code quality and best practices
  • Check for security and performance issues
  • Evaluate test coverage
  • Post review feedback

Summary

This PR makes three improvements to @cubejs-client/core:

  1. Sub-path export for ./format — Exposes formatValue via @cubejs-client/core/format for ESM consumers, avoiding CJS/UMD bundle bloat (aligning with the existing comment at src/index.ts:914).
  2. currency + granularity in TableColumn — Carries these annotation fields through tableColumns() so consumers can use them for formatting without re-parsing annotations.
  3. Boolean type handling in formatValue — Coerces boolean and numeric (0/1) values when type === 'boolean', addressing SQL driver inconsistencies.

Code Quality ✅

  • Clean, well-structured changes. The extractFields destructuring in ResultSet.ts:920 is a natural extension.
  • granularity: granularity?.name correctly unwraps the GranularityAnnotation object to a plain string, matching the TableColumn type contract.
  • JSDoc comments on the new TableColumn fields are clear and helpful.
  • The exports + typesVersions setup in package.json follows the recommended Node.js sub-path export pattern.

Security ✅

No security concerns. The changes are purely formatting/presentation logic with no user input reaching execution paths.

Performance ✅

No performance concerns. The boolean check adds a negligible branch at the top of formatValue. The currency/granularity extraction is a simple destructuring addition.

Testing 🟡

  • The boolean coercion test covers true/false, 1/0, and 'true'/'false' strings — good coverage of the common SQL driver representations.
  • ResultSet.test.ts snapshot updates correctly verify the new fields propagate through tableColumns().
  • Gap: String-encoded '0'/'1' values (which some SQL drivers produce) hit the String(value) fallback and return '0'/'1' instead of 'false'/'true'. This may or may not be intentional — see inline comment with a suggestion.
  • Minor: The formatValue(true, { type: 'boolean' }) assertion is duplicated between the existing "default fallback" test (line 99) and the new "boolean: coerces numeric 0/1" test (line 104).
  • I was unable to run the test suite due to tool permission constraints, so I recommend verifying all tests pass in CI.

Overall

Clean, focused PR. The sub-path export is a good pattern for keeping the CJS bundle lean. The currency/granularity passthrough and boolean formatting are straightforward and well-tested. I left a few minor inline suggestions — nothing blocking.

Comment on lines +21 to +24
"./format": {
"types": "./dist/src/format.d.ts",
"import": "./dist/src/format.js"
}
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.

Nit (non-blocking): The ./format sub-path only exposes types + import — there's no require entry. This means require('@cubejs-client/core/format') will fail for CJS consumers.

I see from src/index.ts:914 that this is intentional to avoid bloating the CJS/UMD bundle — which is a reasonable trade-off. Just flagging that this could surprise users who mix CJS and ESM. Consider adding a brief note in the package README or the JSDoc for formatValue mentioning ESM-only availability via the sub-path.

Comment thread packages/cubejs-client-core/src/format.ts
Comment thread packages/cubejs-client-core/test/format.test.ts
@ovr ovr merged commit 9fee612 into master Apr 15, 2026
12 checks passed
@ovr ovr deleted the fix/client-core-format branch April 15, 2026 13:36
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.03%. Comparing base (9ba2897) to head (24a186e).
⚠️ Report is 2 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (9ba2897) and HEAD (24a186e). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (9ba2897) HEAD (24a186e)
cubesql 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #10692       +/-   ##
===========================================
- Coverage   78.80%   58.03%   -20.78%     
===========================================
  Files         465      215      -250     
  Lines       91944    16683    -75261     
  Branches     3357     3357               
===========================================
- Hits        72458     9682    -62776     
+ Misses      18996     6511    -12485     
  Partials      490      490               
Flag Coverage Δ
cube-backend 58.03% <ø> (ø)
cubesql ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

client:core Issues relating to the JavaScript client SDK javascript Pull requests that update Javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant