feat: Add custom client class support - #4587
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4-next #4587 +/- ##
==========================================
Coverage ? 85.63%
==========================================
Files ? 643
Lines ? 66673
Branches ? 8081
==========================================
Hits ? 57095
Misses ? 9516
Partials ? 62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
czy88840616
left a comment
There was a problem hiding this comment.
Thanks for the PR. The direction is useful, but I think this needs to be narrowed and aligned before merge.
customClientClassis used in Redis/OSS/COS/Consul/ETCD/Sequelize, but the public config types are not updated.
ServiceFactoryConfigOption still only types default / client / clients values as PowerPartial<OPTIONS>, and DataSourceManagerConfigOption only exposes customDataSourceClass. In a normal TypeScript config, users cannot add customClientClass cleanly without casts, so the feature is effectively untyped.
- Sequelize should not introduce a separate
customClientClassAPI.
Sequelize is a DataSourceManager component like TypeORM. Core already has customDataSourceClass, and TypeORM uses that name. If Sequelize adds customClientClass, then customDataSourceClass is silently ignored for Sequelize while the new field is not part of the DataSourceManager config type. This creates an unnecessary inconsistency between ORM components.
I suggest changing Sequelize to use customDataSourceClass and adding tests for it.
- The PR currently applies the same extension to OSS/COS/Consul/ETCD, but the compatible-client contract is unclear.
For vendor-SDK wrapper components like OSS/COS, there may not be a real compatible replacement client. A custom class can also bypass or break assumptions around proxy methods, trace wrapping, and lifecycle behavior. Based on the discussion, I suggest narrowing this PR first to components where the abstraction is clear, such as Sequelize and possibly Redis.
For Redis, please make the supported contract explicit, for example “an ioredis-compatible subclass/wrapper”, and add tests covering normal client creation, ready / error handling, trace binding, and injected service proxy behavior.
Overall recommended scope for this PR:
- Sequelize: support
customDataSourceClass, matching TypeORM. - Redis: only if the custom client is explicitly ioredis-compatible, with tests and config typing.
- Defer OSS/COS/Consul/ETCD unless there is a concrete compatible-client use case and tests for trace/lifecycle behavior.
|
补充一下,上一条 review 里第 3 点我表述得不准确,收回“不建议扩 OSS/COS/Consul/ETCD”这个判断。既然已经有企业内部扩展诉求,这些 vendor SDK wrapper 组件也可以继续做扩展支持。 我这边建议本 PR 重点调整为:
现在代码里读取了
Sequelize 和 TypeORM 一样走
至少覆盖:
整体上我主要 concern 是类型和一致性,不是反对这些组件做扩展。 |
|
感谢反馈!已按要求调整:
另外 createClient 内部暂时保留了 as any,为了不做特别大的代码调整。请再帮忙看看是否还有其他问题? |
There was a problem hiding this comment.
Pull request overview
This PR adds opt-in support for supplying a custom client/data-source class via configuration, allowing Midway’s various ServiceFactory/DataSourceManager-based integrations (Redis/OSS/ETCD/COS/Consul/Sequelize) to instantiate user-provided client implementations while keeping existing factory/service injection and trace-context binding behavior.
Changes:
- Add
customClientClasstoServiceFactoryConfigOptiontyping (core) and implement custom instantiation in multiple package managers. - Add
customDataSourceClassinstantiation support in Sequelize data source manager. - Add test suites validating custom client/data source creation, config merge behavior, and trace binding.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sequelize/src/dataSourceManager.ts | Instantiate Sequelize via customDataSourceClass when provided. |
| packages/sequelize/test/customDataSource.test.ts | Tests for custom Sequelize data source creation and merged config behavior. |
| packages/redis/src/manager.ts | Support customClientClass for Redis client creation. |
| packages/redis/test/customClient.test.ts | Tests for custom Redis clients, injection, error propagation, and tracing. |
| packages/oss/src/manager.ts | Support customClientClass for OSS client creation (with trace binding). |
| packages/oss/test/customClient.test.ts | Tests for custom OSS clients and tracing. |
| packages/etcd/src/manager.ts | Support customClientClass for ETCD client creation (with trace binding). |
| packages/etcd/test/customClient.test.ts | Tests for custom ETCD clients and tracing. |
| packages/cos/src/manager.ts | Support customClientClass for COS client creation (with trace binding). |
| packages/cos/test/customClient.test.ts | Tests for custom COS clients and tracing. |
| packages/consul/src/manager.ts | Support customClientClass for Consul client creation (with trace binding). |
| packages/consul/test/customClient.test.ts | Tests for custom Consul clients, config merge, and tracing. |
| packages/core/src/interface.ts | Extend ServiceFactoryConfigOption to allow customClientClass. |
| packages/core/test/common/serviceFactory.test.ts | Type-level test ensuring customClientClass is accepted by config option types. |
Comments suppressed due to low confidence (1)
packages/redis/src/manager.ts:103
- When using
customClientClass, this code assumes the created client implements EventEmitter semantics (on('ready')/on('error')). If a custom client doesn't exposeon(or if it is already inreadystate and emittedreadysynchronously in its constructor), app startup will throw a confusingclient.on is not a functionerror or hang forever waiting forready. Consider validating the interface and short-circuiting when the client is already ready to make the new extension point safer.
await new Promise<void>((resolve, reject) => {
client.on('ready', () => {
this.logger.info(`[midway:redis] client(${name}) connect success`);
resolve();
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async createClient( | ||
| config: OSSServiceFactoryCreateClientConfigType | ||
| ): Promise<T> { |
| @@ -42,6 +42,13 @@ export class ETCDServiceFactory extends ServiceFactory<Etcd3> { | |||
| protected traceInjector; | |||
|
|
|||
| async createClient(config: IOptions): Promise<Etcd3> { | |||
| @@ -43,6 +43,13 @@ export class COSServiceFactory extends ServiceFactory<COS> { | |||
| protected traceInjector; | |||
|
|
|||
| async createClient(config: COS.COSOptions): Promise<COS> { | |||
| async createClient( | ||
| config: ConsulOptions, | ||
| clientName: string | ||
| ): Promise<InstanceType<typeof Consul>> { |
Checklist
npm testpassesAffected core subsystem(s)
不影响,完全兼容
Description of change
支持自定义客户端扩展
#4586