fix(server-core): correct return type of DialectFactoryFn and ExternalDialectFactoryFn - #11457
Open
AdvMaple wants to merge 2 commits into
Open
fix(server-core): correct return type of DialectFactoryFn and ExternalDialectFactoryFn#11457AdvMaple wants to merge 2 commits into
AdvMaple wants to merge 2 commits into
Conversation
…lDialectFactoryFn Both DialectFactoryFn and ExternalDialectFactoryFn were typed as returning BaseQuery (an instance), but the runtime uses the return value as a constructor: new (queryClass(dbType, dialectClass))(compilers, queryOptions) The dialectFactory is expected to return a class (subclass of BaseQuery), not an instance. PR cube-js#1997 improved the type from 'any' to 'BaseQuery' but the correct type is 'typeof BaseQuery'. Evidence: - CompilerApi.getDialectClass() returns what dialectFactory returned and passes it directly to QueryBuilder.queryClass(), which calls new on it. - The contributing guide (CONTRIBUTING.md) shows dialectClass() returning FooQuery (a class reference), not new FooQuery(). - BaseQuery JSDoc: 'You should never instantiate this class manually.' - All built-in dialects (MysqlQuery, PostgresQuery, etc.) are registered as class constructors, not instances. This change makes custom dialect registration type-safe without requiring casts or ts-ignore suppression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Writing a custom Cube.js dialect produces a TypeScript error with no clean fix.
DialectFactoryFnis typed to return aBaseQueryinstance, but at runtime Cube.js calls the return value withnew. So a correct factory like(ctx) => HanaQueryfails type-checking, and the only escape hatch is:That cast is wrong — and misleading to readers.
Root Cause
#1997 tightened the type from
anytoBaseQuery, which was good — butBaseQueryandtypeof BaseQuerymean different things:BaseQuerytypeof BaseQuerynewThe factory must return a constructor, not an instance.
Context
Discovered while vendoring a local SAP HANA driver (the original PR #5648 was never merged).
dialectFactory: (ctx) => HanaQueryis the correct pattern — it matches every built-in dialect — but it triggers the error above.BaseQueryhas 295+ instance properties populated at construction time by the framework. A custom dialect author cannot satisfy that shape directly;extends BaseQueryis the right approach. The error is purely a type annotation bug.Fix
With this change,
dialectFactory: (ctx) => MyCustomQuerytype-checks naturally for anyMyCustomQuery extends BaseQuery— no casts needed.Testing
Type-only change. No runtime behaviour altered. TypeScript compilation of
packages/cubejs-server-corepasses.