fix: resolve async factory services in dispatcher and all framework adapters#859
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
… all adapters Resolves runtime error "Service 'X' is async - use await" when services like auth and analytics are registered with async factories. - http-dispatcher.ts: getService() and getObjectQLService() now prefer kernel.getServiceAsync() over sync kernel.getService() - All 7 adapters (Express, Fastify, Hono, Next.js, SvelteKit, NestJS, Nuxt) updated to use getServiceAsync() for auth service resolution with graceful fallback to sync path Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 评估是否所有服务都有类似的问题,仔细检查是否已经完成了所有的开发并测试 |
…sync tests
- Fixed 6 occurrences of this.kernel.context.getService('protocol') in
handleMetadata and handleUi — these used the sync path that throws
for async factory services, same root cause as auth/analytics.
- Introduced resolveService() helper that all service lookups now use:
getServiceAsync → getService → context.getService → services Map
with null-check fallthrough at each step.
- Simplified getObjectQLService() to use resolveService() internally.
- Added 7 new dispatcher tests for getServiceAsync preferred path
covering analytics, auth, automation, file-storage, protocol,
null fallthrough, and error fallthrough.
- Added getServiceAsync tests for Hono, Next.js, and SvelteKit adapters.
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Completed a thorough audit of all service resolution paths. Found and fixed additional issues beyond auth/analytics: Previously missed —
Fix: Introduced All services now covered:
Test coverage added (commit
|
There was a problem hiding this comment.
Pull request overview
Fixes runtime failures when services are registered via async factories by resolving services through an async-first path in the dispatcher and framework adapters.
Changes:
- Added
HttpDispatcher.resolveService()to preferkernel.getServiceAsync()and updated dispatcher service lookups (includingprotocol) to use it. - Updated all framework adapters’ auth resolution to prefer
getServiceAsync()with sync fallback. - Added/updated tests for dispatcher and selected adapters; updated
ROADMAP.mdentry.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/src/http-dispatcher.ts | Introduces resolveService() and routes protocol/objectql/core services through it. |
| packages/runtime/src/http-dispatcher.test.ts | Adds tests for async-first resolution and fallthrough behavior. |
| packages/adapters/express/src/index.ts | Switches auth service lookup to async-first. |
| packages/adapters/fastify/src/index.ts | Switches auth service lookup to async-first. |
| packages/adapters/hono/src/index.ts | Switches auth service lookup to async-first. |
| packages/adapters/hono/src/hono.test.ts | Adds adapter tests covering getServiceAsync auth resolution and fallthrough. |
| packages/adapters/nextjs/src/index.ts | Switches auth service lookup to async-first. |
| packages/adapters/nextjs/src/nextjs.test.ts | Adds adapter tests covering getServiceAsync auth resolution and fallthrough. |
| packages/adapters/sveltekit/src/index.ts | Switches auth service lookup to async-first. |
| packages/adapters/sveltekit/src/sveltekit.test.ts | Adds adapter tests covering getServiceAsync auth resolution and fallthrough. |
| packages/adapters/nestjs/src/index.ts | Switches auth service lookup to async-first. |
| packages/adapters/nuxt/src/index.ts | Switches auth service lookup to async-first. |
| ROADMAP.md | Updates roadmap note to reflect adapter coverage. |
| } catch { | ||
| // Service not registered — fall through to dispatcher | ||
| authService = null; |
There was a problem hiding this comment.
This catch-all treats any error during auth service resolution as "service not registered" and falls back to the dispatcher. That fixes the async-factory sync-call case, but it can also mask genuine auth factory/initialization failures. Consider only swallowing expected lookup errors and rethrow unexpected errors so failures remain observable.
| } catch { | |
| // Service not registered — fall through to dispatcher | |
| authService = null; | |
| } catch (err) { | |
| // Only treat expected "service not registered" lookup errors as absence. | |
| const isLookupError = | |
| err instanceof Error && | |
| typeof err.message === 'string' && | |
| /not\s+(registered|found)/i.test(err.message); | |
| if (isLookupError) { | |
| // Service not registered — fall through to dispatcher | |
| authService = null; | |
| } else { | |
| // Unexpected error during auth service resolution: rethrow so it remains observable | |
| throw err; | |
| } |
| } catch { | ||
| // Service not registered — fall through to dispatcher | ||
| authService = null; |
There was a problem hiding this comment.
This catch-all treats any error during auth service resolution as "service not registered" and falls back to the dispatcher. That can mask genuine auth factory/initialization failures. Consider only swallowing expected lookup errors (not found / "is async") and rethrow unexpected errors so failures surface as 500s.
| } catch { | |
| // Service not registered — fall through to dispatcher | |
| authService = null; | |
| } catch (err: any) { | |
| const message = typeof err?.message === 'string' ? err.message : ''; | |
| const lowerMessage = message.toLowerCase(); | |
| const isLookupError = | |
| lowerMessage.includes('not registered') || | |
| lowerMessage.includes('not found') || | |
| lowerMessage.includes('is async'); | |
| if (isLookupError) { | |
| // Service not registered or only available asynchronously — fall through to dispatcher | |
| authService = null; | |
| } else { | |
| // Unexpected error — rethrow so it surfaces as a 500 | |
| throw err; | |
| } |
| } catch { | ||
| // Service not registered — fall through to legacy dispatcher | ||
| authService = null; |
There was a problem hiding this comment.
This catch-all treats any error during auth service resolution as "service not registered" and falls back to the dispatcher, which can mask real auth factory/initialization failures. Consider only swallowing expected lookup errors (not found / "is async") and rethrow unexpected errors so failures surface correctly.
| } catch { | |
| // Service not registered — fall through to legacy dispatcher | |
| authService = null; | |
| } catch (error: any) { | |
| const message = typeof error?.message === 'string' ? error.message : ''; | |
| const isNotRegistered = | |
| message.includes('not registered') || message.includes('not found'); | |
| const isAsyncHint = message.includes('is async'); | |
| if (isNotRegistered || isAsyncHint) { | |
| // Auth service not registered or only available asynchronously — | |
| // fall through to legacy dispatcher. | |
| authService = null; | |
| } else { | |
| // Unexpected error during auth service resolution — surface it. | |
| throw error; | |
| } |
| if (typeof kernel.getServiceAsync === 'function') { | ||
| authService = await kernel.getServiceAsync<AuthService>('auth'); | ||
| } else if (typeof kernel.getService === 'function') { | ||
| authService = kernel.getService<AuthService>('auth'); | ||
| } |
There was a problem hiding this comment.
NestJS adapter has tests, but they don’t cover the new auth resolution branch that prefers kernel.getServiceAsync(). Add a test that supplies getServiceAsync('auth') and asserts it is called (and a fallthrough test when it throws/returns null) to prevent regressions.
| } catch { | ||
| // Service not registered — fall through to dispatcher |
There was a problem hiding this comment.
This catch-all treats any error during auth service resolution as "service not registered" and falls back to the dispatcher. That can mask genuine auth factory/initialization failures. Consider only swallowing expected lookup errors (e.g. not-found / "is async - use await") and rethrow unexpected errors so failures surface as 500s.
| } catch { | |
| // Service not registered — fall through to dispatcher | |
| } catch (err) { | |
| const message = | |
| err instanceof Error | |
| ? err.message | |
| : typeof err === 'string' | |
| ? err | |
| : String(err); | |
| const isNotRegistered = | |
| message.includes('not registered') || | |
| message.includes('not found') || | |
| message.includes('service "auth"'); | |
| const isAsyncHint = | |
| message.includes('is async - use await') || | |
| message.includes('use getServiceAsync'); | |
| if (!isNotRegistered && !isAsyncHint) { | |
| // Unexpected error during auth service resolution — surface as 500 | |
| throw err; | |
| } | |
| // Service not registered / not available — fall through to dispatcher |
| if (typeof options.kernel.getServiceAsync === 'function') { | ||
| authService = await options.kernel.getServiceAsync<AuthService>('auth'); | ||
| } else if (typeof options.kernel.getService === 'function') { | ||
| authService = options.kernel.getService<AuthService>('auth'); | ||
| } |
There was a problem hiding this comment.
Nuxt adapter has tests, but they don’t cover the new auth resolution branch that prefers kernel.getServiceAsync(). Add a test that supplies getServiceAsync('auth') and asserts the auth handler is used, plus a fallthrough test when getServiceAsync throws/returns null.
| try { | ||
| const svc = await this.kernel.getServiceAsync(name); | ||
| if (svc != null) return svc; | ||
| } catch { | ||
| // Service not registered or async resolution failed — fall through |
There was a problem hiding this comment.
resolveService() currently catches all errors from kernel.getServiceAsync() and silently falls through. That can mask real service factory failures (misconfig/connection errors) and make the dispatcher behave as if the service is simply absent. Consider only falling through on "not found" (or other explicitly non-fatal) errors and rethrow unexpected errors so factory failures surface as 500s.
| } catch { | ||
| // Service not registered or sync resolution threw "is async" — fall through |
There was a problem hiding this comment.
resolveService() also swallows all errors from kernel.getService() (sync). If getServiceAsync is unavailable and getService throws "Service 'X' is async - use await", swallowing it will likely return undefined from the map and hide the actionable error. Consider detecting the "is async" case and rethrowing (or wrapping) when there is no async API available.
| } catch { | |
| // Service not registered or sync resolution threw "is async" — fall through | |
| } catch (error: unknown) { | |
| // If there is no async API and the sync call explicitly reports that the | |
| // service is async ("Service 'X' is async - use await"), surface that | |
| // error instead of swallowing it, as it is an actionable misconfiguration. | |
| if (typeof this.kernel.getServiceAsync !== 'function') { | |
| const err = error instanceof Error ? error : undefined; | |
| if (err && /is async/i.test(err.message) && /use await/i.test(err.message)) { | |
| throw err; | |
| } | |
| } | |
| // Otherwise, service not registered or other sync resolution error — fall through |
| if (typeof options.kernel.getServiceAsync === 'function') { | ||
| authService = await options.kernel.getServiceAsync<AuthService>('auth'); | ||
| } else if (typeof options.kernel.getService === 'function') { | ||
| authService = options.kernel.getService<AuthService>('auth'); | ||
| } |
There was a problem hiding this comment.
The Express adapter changed auth resolution to prefer kernel.getServiceAsync(), but the adapter test suite doesn’t cover this branch. Add a test that supplies getServiceAsync('auth') and asserts it is used (and another that asserts fallthrough when getServiceAsync throws/returns null) to prevent regressions.
| if (typeof options.kernel.getServiceAsync === 'function') { | ||
| authService = await options.kernel.getServiceAsync<AuthService>('auth'); | ||
| } else if (typeof options.kernel.getService === 'function') { | ||
| authService = options.kernel.getService<AuthService>('auth'); | ||
| } |
There was a problem hiding this comment.
Fastify adapter tests exist, but they don’t cover the new auth resolution branch that prefers kernel.getServiceAsync(). Add a test that provides getServiceAsync('auth') and asserts it is used, plus a fallthrough test when getServiceAsync throws/returns null.
kernel.getService('auth'),kernel.getService('analytics'), andkernel.context.getService('protocol')throw"Service 'X' is async - use await"when registered via async factory (PluginLoader). The dispatcher's internalgetService(), directprotocolservice lookups, and all adapter auth lookups used the sync path, which the kernel rejects for Promise-returning factories.Dispatcher (
http-dispatcher.ts)resolveService(name)helper with full fallback chain:getServiceAsync→getService(sync) →context.getService→ services Map. Each step checks for non-null results before returning, ensuring proper fallthrough.getService()delegates toresolveService()for all CoreServiceName lookups.getObjectQLService()simplified to useresolveService().this.kernel.context.getService('protocol')inhandleMetadata()andhandleUi()— these used the same sync path that throws for async factory services.Framework Adapters (all 7)
getServiceto async-first pattern with graceful fallback:All affected services
auth,analytics,metadata,file-storage,automation— viagetService()→resolveService()objectql— viagetObjectQLService()→resolveService()protocol— 6 directcontext.getService()calls replaced withresolveService('protocol')Tests
getServiceAsyncpreferred path (analytics, auth, automation, file-storage, protocol, null fallthrough, error fallthrough)getServiceAsyncadapter tests for Hono, Next.js, SvelteKitFully backward-compatible — falls through to sync path when
getServiceAsyncis unavailable.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.