fix: add missing SiteConfig service override typings and definitions by vkumar-sonata · Pull Request #281 · openedx/frontend-base

vkumar-sonata · 2026-07-21T10:02:37Z

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:

loggingService
analyticsService
authService

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

TypeScript compilation succeeds
No runtime changes introduced

Context

Discovered while attempting to configure a custom logging service.

LLM usage notice

Built with assistance from Copilot.

Closes#293

arbrandes

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:

exporttypeLoggingServiceClass=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.

arbrandes · 2026-08-05T11:42:20Z

+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>,
+}


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.

@arbrandes Fixed. Removed the hand-written LoggingService, AnalyticsService and AuthService interfaces that had incorrect method names. Replaced them with
LoggingService re-exported directly from runtime/logging/types.ts which already had the correct serviceshape in runtime/logging/interface.js
AnalyticsService defined in the new runtime/analytics/types.ts with method names derived from the serviceshape in runtime/analytics/interface.js
AuthService defined in the new runtime/auth/types.ts with method names derived from serviceshape in runtime/auth/interface.js

arbrandes · 2026-08-05T11:42:20Z

+ loggingService: LoggingService,
+ analyticsService: AnalyticsService,
+ authService: AuthService,


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:
exporttypeExternalScriptLoaderClass=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).

@arbrandes Fixed. OptionalSiteConfig keys now use LoggingServiceClass, AnalyticsServiceClass and AuthServiceClass constructor types following the same pattern as the existing ExternalScriptLoaderClass.
Added the test-types/site-config-service-overrides.typecheck.ts which assigns NewRelicLoggingService, SegmentAnalyticsService and AxiosJwtAuthService directly to a typed SiteConfig const and typechecks cleanly with npx tsc --noEmit --skipLibCheck --moduleResolution node test-types/site-config-service-overrides.typecheck.ts (no output = no errors).
NOTE: running npx tsc --noEmit across full project hits a pre-existing TS2209 error on main (rootDir ambiguity from package.json export map) that is unrelated to this PR. Confirmed pre-existing by running on a clean git stash.

arbrandes

A few more change requests, if you don't mind. Thanks for bearing with me!

arbrandes · 2026-08-20T14:31:15Z

+ setAuthenticatedUser(authUser: Record<string, unknown>): void,
+ fetchAuthenticatedUser(options?: Record<string, unknown>): Promise<Record<string, unknown> | null>,
+ ensureAuthenticatedUser(redirectUrl?: string): Promise<Record<string, unknown>>,
+ hydrateAuthenticatedUser(): Promise<null>,


Should return Promise<void>, not Promise<null>.
Neither implementation resolves to null, and as written the type rejects MockAuthService (TS2419: Type 'void' is not assignable to type 'Promise<null>'). MockAuthService.js:270 is a jest.fn() wrapping a callback with no return; AxiosJwtAuthService passes only on a stale JSDoc @returns {Promise<null>} above AxiosJwtAuthService.js:293, while its body returns undefined. runtime/auth/interface.js:249-250 awaits the result and discards it.

arbrandes · 2026-08-20T14:31:15Z

+ getAuthenticatedUser(): Record<string, unknown> | null,
+ setAuthenticatedUser(authUser: Record<string, unknown>): void,
+ fetchAuthenticatedUser(options?: Record<string, unknown>): Promise<Record<string, unknown> | null>,
+ ensureAuthenticatedUser(redirectUrl?: string): Promise<Record<string, unknown>>,


Use User (types.ts:153) instead of Record<string, unknown> for the user-data methods - SiteContext.tsx:23 already does this. It is technically too strict by exactly one field, avatar, but it looks like this is bug in User: feel free to include the fix here (making avatar optional in the type).

arbrandes · 2026-08-20T14:31:15Z

@@ -0,0 +1,7 @@
+export interface AnalyticsService {
+ sendTrackingLogEvent(eventName: string, properties: object): Promise<void>,


Should return Promise<unknown>, not Promise<void>.
Promise<void> rejects the reference implementation's own shape - SegmentAnalyticsService.js:140 does return this.httpClient.post(...). It passes today only because that file is untyped JS, so httpClient is implicitly any; the same service written in TypeScript would fail.

arbrandes · 2026-08-20T14:31:15Z

+import { SiteConfig } from '../types';
+import NewRelicLoggingService from '../runtime/logging/NewRelicLoggingService';
+import SegmentAnalyticsService from '../runtime/analytics/SegmentAnalyticsService';
+import AxiosJwtAuthService from '../runtime/auth/AxiosJwtAuthService';
+
+const config: SiteConfig = {
+ loggingService: NewRelicLoggingService,
+ analyticsService: SegmentAnalyticsService,
+ authService: AxiosJwtAuthService,
+ siteId: '',
+ siteName: '',
+ baseUrl: '',
+ lmsBaseUrl: '',
+ loginUrl: '',
+ logoutUrl: '',
+}
+
+export default config;


Remove test-types/ and the eslint.config.js:16 ignore.
The real fix is typing runtime/initialize.js. If that were TypeScript, getSiteConfig().loggingService would tie the declarations to real usage. But this is obviously out of scope, here.

arbrandes · 2026-08-20T14:31:15Z

 'test-site/*',
 'config/*',
 'docs/*',
+ 'test-types/*',


This ignore comes back out along with test-types/ - see the comment on the fixture file.

arbrandes · 2026-08-20T14:31:15Z

 export type LocalizedMessages = Record<string, Record<string, string>>;
 export type SiteMessages = LocalizedMessages[];

+export type { LoggingService, AnalyticsService, AuthService };


Export the new types from their barrels with export type * from './types', as runtime/slots/index.ts:2 does, rather than re-exporting here. Both reach consumers; the barrel keeps the layering consistent.

arbrandes · 2026-08-20T14:31:16Z


+export type { LoggingService, AnalyticsService, AuthService };
+
+// Logging instantiated


Drop the // Logging instantiated / // Analytics instantiated / // Auth instantiated comments here and at 72 and 79 - these are constructor types, nothing is instantiated. ExternalScriptLoaderClass at types.ts:45 carries no comment.

arbrandes · 2026-08-20T14:31:16Z

+ config: {
+ baseUrl: string,
+ lmsBaseUrl: string,
+ loginUrl: string,
+ logoutUrl: string,
+ refreshAccessTokenApiPath: string,
+ accessTokenCookieName: string,
+ csrfTokenApiPath: string,
+ },
+ loggingService: object,
+ middleware?: unknown[],


Use config: SiteConfig like the other two rather than the inlined seven-field literal - initialize() passes the whole getSiteConfig(). middleware? on line 91 is also always supplied (it defaults to [] in the initialize signature), so it isn't optional.

arbrandes · 2026-08-20T14:31:16Z

+ accessTokenCookieName: string,
+ csrfTokenApiPath: string,
+ },
+ loggingService: object,


Use LoggingService, not object - line 75 already does for the same value, and initialize() passes getLoggingService() to both.

fix: add missing SiteConfig service override definitions
ea6e34c

diana-villalvazo-wgu marked this pull request as ready for review July 22, 2026 15:56

diana-villalvazo-wgu requested a review from arbrandes July 22, 2026 15:58

arbrandes requested changes Aug 5, 2026
View reviewed changes

fix: correct service constructor types and add typecheck fixture
3195cdc

vkumar-sonata requested a review from arbrandes August 7, 2026 06:46

arbrandes mentioned this pull request Aug 20, 2026
SiteConfig type is missing the logging, analytics and auth service overrides #293
Open

arbrandes requested changes Aug 20, 2026
View reviewed changes

		@@ -0,0 +1,7 @@
		export interface AnalyticsService {
		sendTrackingLogEvent(eventName: string, properties: object): Promise<void>,


		export type { LoggingService, AnalyticsService, AuthService };

		// Logging instantiated

Uh oh!

Conversation

vkumar-sonata commented Jul 21, 2026• edited by arbrandes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fix

Validation

Context

LLM usage notice

Uh oh!

arbrandes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arbrandes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vkumar-sonata commented Jul 21, 2026•
edited by arbrandes
Loading