fix: add missing SiteConfig service override typings and definitions - #281
fix: add missing SiteConfig service override typings and definitions#281vkumar-sonata wants to merge 1 commit into
Conversation
arbrandes
left a comment
arbrandes
left a comment
There was a problem hiding this comment.
The problem this PR identifies is real. initialize() reads loggingService, analyticsService, and authService off the site config, but OptionalSiteConfig never declared them, so setting one in a site.config.tsx typed as SiteConfig is an excess-property error.
However, the interfaces added here don't fix it. They declare instance shapes where the runtime requires constructors, and the method names don't correspond to any service in the repo.
The direction that would work is an instance contract per service plus a constructor type wrapping it, with the config keys referencing the constructor type. Logging is the cheap illustration, since runtime/logging/types.ts already has the contract:
export type LoggingServiceClass = new (options: { config: SiteConfig }) => LoggingService;For the other two, the serviceShape blocks in configureAnalytics and configureAuth are the authoritative method lists.
One smaller pointer. Co-locating each contract with its service rather than in root types.ts would match how SlotOperation is handled at types.ts:4. The tradeoff is reach: root types.ts is already public via index.ts, whereas none of the logging, analytics, or auth barrels export types, so co-locating means wiring that up as well.
On validation: a successful build doesn't exercise any of this. The repo typechecks either way because nothing here assigns to those keys, and consumer builds run ts-loader with transpileOnly: true (tools/webpack/common-config/all/getCodeRules.ts:16-18), so a green consumer build proves nothing either. A site.config.tsx that sets one of these to a real service class, typechecked and then booted, would.
| export interface LoggingService { | ||
| debug?(message: string, meta?: Record<string, unknown>): void, | ||
| info?(message: string, meta?: Record<string, unknown>): void, | ||
| warn?(message: string, meta?: Record<string, unknown>): void, | ||
| error?(message: string | Error, meta?: Record<string, unknown>): void, | ||
| } | ||
|
|
||
| // Generic analytics contract | ||
| export interface AnalyticsService { | ||
| identify?(userId: string | number, traits?: Record<string, unknown>): void, | ||
| track(event: string, properties?: Record<string, unknown>): void, | ||
| page?(name?: string, properties?: Record<string, unknown>): void, | ||
| reset?(): void, | ||
| } | ||
|
|
||
| // Generic auth contract | ||
| export interface AuthService { | ||
| isAuthenticated(): boolean | Promise<boolean>, | ||
| getAccessToken?(): string | null | Promise<string | null>, | ||
| login?(redirectUrl?: string): void | Promise<void>, | ||
| logout?(redirectUrl?: string): void | Promise<void>, | ||
| getCurrentUser?(): User | null | Promise<User | null>, | ||
| } |
There was a problem hiding this comment.
None of the three interfaces match the contracts the runtime validates against.
For logging, runtime/logging/types.ts:1-4 already defines LoggingService, with the correct shape (logInfo, logError). This is a second, contradictory definition of the same name, though unfortunately it is the one that becomes public API. configureLogging validates the instance against { logInfo, logError } (runtime/logging/interface.js:34-37), which is what NewRelicLoggingService:132-152 and MockLoggingService:14-21 implement. No logging service in the repo has debug, info, warn, or error.
For analytics, configureAnalytics requires sendTrackingLogEvent, identifyAuthenticatedUser, identifyAnonymousUser, sendTrackEvent, and sendPageEvent (runtime/analytics/interface.js:42-48), which is what SegmentAnalyticsService:133-234 implements. There's no track, page, identify, or reset. track being non-optional also makes this a hard error rather than a weak-type warning: analyticsService: SegmentAnalyticsService fails with TS2741: Property 'track' is missing.
For auth, configureAuth requires eleven methods (runtime/auth/interface.js:73-85). AxiosJwtAuthService:111-310 has neither isAuthenticated nor getCurrentUser, so authService: AxiosJwtAuthService fails with TS2741: Property 'isAuthenticated' is missing. That class is a perfectly good service at runtime; it's the declared type that rejects it.
| loggingService: LoggingService, | ||
| analyticsService: AnalyticsService, | ||
| authService: AuthService, |
There was a problem hiding this comment.
These keys hold service classes, not instances. Each configure* function instantiates what it is given: runtime/logging/interface.js:46, runtime/analytics/interface.js:60, runtime/auth/interface.js:97. The initialize() defaults are the classes themselves (runtime/initialize.js:272-274).
Typing them as instances means a consumer who satisfies the type with an object literal gets a TypeError.
types.ts:42 already has the correct pattern for this:
export type ExternalScriptLoaderClass = new (data: { config: AppConfig }) => ExternalScriptLoader;Constructor options differ per service: logging gets { config } (initialize.js:305-307), auth gets { loggingService, config, middleware } (initialize.js:319-323), analytics gets { config, loggingService, httpClient } (initialize.js:329-333).
2 participants
Description
This PR aligns the SiteConfig TypeScript definitions with the existing runtime implementation.
The runtime initialize() function supports overriding the default service implementations through properties defined on SiteConfig:
However, these properties are currently not represented in the SiteConfig TypeScript definitions. As a result, consumers receive TypeScript compilation errors when attempting to register supported service overrides through site configuration.
Fix
Add the missing service override definitions to OptionalSiteConfig so that the TypeScript API matches the existing runtime behavior.
Validation
Context
Discovered while attempting to configure a custom logging service.
LLM usage notice
Built with assistance from Copilot.