Make @hyperdx/common-utils forward-compatible with @clickhouse/client 1.23#2500
Conversation
🦋 Changeset detectedLatest commit: 5f317f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR makes
Confidence Score: 4/5The core forward-compat work in node.ts and browser.ts is correct and well-structured, but ProxyClickhouseClient in the CLI package was not fully migrated and will still fail to compile under @clickhouse/client 1.23. The two 1.23-compat patterns introduced in node.ts (getClient() narrowing + processClickhouseSettings() cast) were not carried over to ProxyClickhouseClient in packages/cli/src/api/client.ts, which creates a Node client the same way. Without those two changes, the CLI's ClickHouse client will produce the same type errors under 1.23 that this PR is designed to eliminate. packages/cli/src/api/client.ts — ProxyClickhouseClient needs a getClient() narrowing override and an Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class BaseClickhouseClient {
+client: WebClickHouseClient | NodeClickHouseClient
+processClickhouseSettings() ClickHouseSettings [from client-common]
+getClient() WebClickHouseClient | NodeClickHouseClient
+query()
}
class ClickhouseClient_node {
+getClient() NodeClickHouseClient [NEW override]
+__query() cast processClickhouseSettings as NodeClickHouseSettings
}
class ClickhouseClient_browser {
+getClient() WebClickHouseClient [NEW override]
+__query() cast processClickhouseSettings as WebClickHouseSettings
}
class ProxyClickhouseClient {
+createClient from @clickhouse/client
+__query() MISSING getClient() override
+__query() MISSING processClickhouseSettings cast
}
BaseClickhouseClient <|-- ClickhouseClient_node : extends node.ts
BaseClickhouseClient <|-- ClickhouseClient_browser : extends browser.ts
BaseClickhouseClient <|-- ProxyClickhouseClient : extends cli/client.ts
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
classDiagram
class BaseClickhouseClient {
+client: WebClickHouseClient | NodeClickHouseClient
+processClickhouseSettings() ClickHouseSettings [from client-common]
+getClient() WebClickHouseClient | NodeClickHouseClient
+query()
}
class ClickhouseClient_node {
+getClient() NodeClickHouseClient [NEW override]
+__query() cast processClickhouseSettings as NodeClickHouseSettings
}
class ClickhouseClient_browser {
+getClient() WebClickHouseClient [NEW override]
+__query() cast processClickhouseSettings as WebClickHouseSettings
}
class ProxyClickhouseClient {
+createClient from @clickhouse/client
+__query() MISSING getClient() override
+__query() MISSING processClickhouseSettings cast
}
BaseClickhouseClient <|-- ClickhouseClient_node : extends node.ts
BaseClickhouseClient <|-- ClickhouseClient_browser : extends browser.ts
BaseClickhouseClient <|-- ProxyClickhouseClient : extends cli/client.ts
|
The coding agent session tested the changes in this PR against the 1.23 here: https://github.com/hyperdxio/hyperdx/sessions/09fe9533-5802-414b-892d-0b6f057a7c20 If that's actually the case, I'll iterate on it until the gating PR turns green. |
E2E Test Results✅ All tests passed • 218 passed • 3 skipped • 1500s
Tests ran across 4 shards in parallel. |
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
There was a problem hiding this comment.
Pull request overview
This PR updates HyperDX’s ClickHouse client type imports so @hyperdx/common-utils (and the CLI) remain compatible with both the currently pinned @clickhouse/client* and the newer 1.23+ packaging where @clickhouse/client-common is deprecated and type identity can diverge.
Changes:
- Swap ClickHouse type imports to come from the same platform package as the runtime client (
@clickhouse/clientfor node/tests/cli,@clickhouse/client-webfor browser). - Add subclass
getClient()narrowing in the node/browser common-utils clients to avoidWeb|Nodeunion type issues. - Remove
@clickhouse/client-commonfrom the CLI package and add a changeset for patch releases.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Drops @clickhouse/client-common from the CLI workspace lock entry. |
| packages/common-utils/src/clickhouse/node.ts | Moves CH types to @clickhouse/client and adds node-specific getClient() narrowing + settings cast. |
| packages/common-utils/src/clickhouse/browser.ts | Moves CH types to @clickhouse/client-web and adds web-specific getClient() narrowing + settings cast. |
| packages/common-utils/src/tests/queryChartConfig.int.test.ts | Updates ClickHouseClient import to @clickhouse/client. |
| packages/common-utils/src/tests/metadata.int.test.ts | Updates ClickHouseClient import to @clickhouse/client. |
| packages/common-utils/src/tests/clickhouse.test.ts | Updates ResponseJSON import to @clickhouse/client. |
| packages/cli/src/api/client.ts | Updates CH type imports to @clickhouse/client. |
| packages/cli/package.json | Removes @clickhouse/client-common devDependency. |
| .changeset/clickhouse-client-forward-compat.md | Adds patch changeset for @hyperdx/common-utils and @hyperdx/cli. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| clickhouseSettings = (await this.processClickhouseSettings({ | ||
| externalClickhouseSettings, | ||
| connectionId, | ||
| }); | ||
| })) as ClickHouseSettings; |
| clickhouseSettings = (await this.processClickhouseSettings({ | ||
| connectionId, | ||
| externalClickhouseSettings, | ||
| }); | ||
| })) as ClickHouseSettings; |
| import type { | ||
| BaseResultSet, | ||
| ClickHouseSettings, | ||
| DataFormat, | ||
| } from '@clickhouse/client-common'; | ||
| } from '@clickhouse/client'; |
Summary
Discovered in:
@clickhouse/clientpackage beta version #1553 by @peter-leonov-chIn
@clickhouse/client*1.23,@clickhouse/client-commonis deprecated:@clickhouse/client(Node) and@clickhouse/client-web(Web) no longer depend on it — each now bundles and re-exports its own copy of the shared types. This repo imported types fromclient-commonbut got its runtime from the platform packages, so under 1.23 the two same-namedClickHouseSettingsaliases become distinct nominal types (their index signature references a private-memberedSettingsMapclass), breaking thecommon-utilsdeclaration build and cascading into every downstream job.This change repoints imports so each file's ClickHouse types come from the same platform package as its runtime, making the repo compile against both the current pin and 1.23. No
@clickhouse/client*version bumps — the actual upgrade can land later with no further code changes.clickhouse/browser.ts(web runtime) →@clickhouse/client-webclickhouse/node.ts(node runtime) →@clickhouse/client__tests__/clickhouse.test.ts,metadata.int.test.ts,queryChartConfig.int.test.ts→@clickhouse/client(matches the client they instantiate; also clears theclientClickHouseSettingsprivate-property errors)packages/cli/src/api/client.ts→@clickhouse/client; dropped the now-unused@clickhouse/client-commondep from clipackage.jsonclient-common—clickhouse/index.tsandcore/metadata.ts. These are cross-platform/barrel modules, andResponseHeadersis not re-exported by the platform packages at the pinned 1.12.1, so migrating them would break the current build. This is the only remaining (justified)client-commonusage.BaseClickhouseClientfeeds one platform-neutral settings object into both aWebClickHouseClient | NodeClickHouseClientunion and both subclasses, whose settings types diverge under 1.23. Bridged with a single typedgetClient()override per subclass plus a neutral→platform cast onprocessClickhouseSettings()— type-only, following the file's existing// client library type mismatchconvention.The only lockfile delta is the cli
client-commonremoval; no version entries change.Screenshots or video
N/A — non-UI change.
How to test on Vercel preview
N/A — non-UI change.
References
@clickhouse/clientpackage beta version #1553 (experimental 1.23 bump that surfaced the failure; not landing)