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:

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.

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.

Fixed. Changed return type from Promise<null> to Promise<void>.

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).

Fixed. Replaced Record<string, unknown> with User for getAuthenticatedUser, setAuthenticatedUser, fetchAuthenticatedUser & ensureAuthenticatedUser. Also, made avatar optional in the User interface as suggested.

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.

Fixed. Changed return type from Promise<void> to Promise<unknown>.

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.

Removed. Deleted the test-types/ folder and the corresponding eslint.config.js ignore entry.

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.

Fixed. Moved the re-exports to the runtime barrel files using export type * from './types' in runtime/logging/index.ts, runtime/analytics/index.ts and runtime/auth/index.ts.

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.

Fixed. Replaced the inlined config literal with config: SiteConfig to match the other two service class types and reflect that initialize() passes the whole getSiteConfig(). Also made middleware non-optional since initialize() always supplies it, defaulting to [].

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.

Fixed. Changed loggingService: object to loggingService: LoggingService in AuthServiceClass, consistent with AnalyticsServiceClass.

vkumar-sonata · 2026-08-31T06:06:33Z

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

@arbrandes Acknowledged and I have made the suggested changes. Please review the changes.

arbrandes

Almost there! Just a few typing and linting adjustments. Thanks again!

arbrandes · 2026-09-01T15:51:17Z

+export type AuthServiceClass = new (options: {
+  config: SiteConfig,
+  loggingService: LoggingService,
+  middleware: unknown[],
+}) => AuthService;


authService: AxiosJwtAuthService still doesn't compile. Replace the seven @param {string} options.config.* lines at AxiosJwtAuthService.js:33-44 with @param {import('../../types').SiteConfig} options.config, and add @param {Array} [options.middleware].

That JSDoc types the constructor's options.config as an object with seven required strings. SiteConfig has refreshAccessTokenApiPath, accessTokenCookieName and csrfTokenApiPath as optional, so config: SiteConfig on line 77 isn't assignable to it:

error TS2322: Type 'typeof AxiosJwtAuthService' is not assignable to type 'AuthServiceClass'. Types of construct signatures are incompatible. The types of 'config.refreshAccessTokenApiPath' are incompatible between these types. Type 'string | undefined' is not assignable to type 'string'.

arbrandes · 2026-09-01T15:51:17Z

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


Promise<void> is right, but the implementation's annotation wasn't updated to match: change AxiosJwtAuthService.js:291 to @returns {Promise<void>}.

The body returns undefined, so the stale @returns {Promise<null>} makes the instance fail AuthService with Type 'Promise<null>' is not assignable to type 'Promise<void>'. With this and the constructor JSDoc fixed, AxiosJwtAuthService satisfies both AuthServiceClass and AuthService.

arbrandes · 2026-09-01T15:51:17Z

  setAuthenticatedUser
 } from './interface';
 export { default as MockAuthService } from './MockAuthService';
+export type * from './types';


These don't reach consumers. Add type LoggingService, type AnalyticsService and type AuthService to the named export lists in runtime/index.ts, following the type IntlConfig pattern at runtime/index.ts:77.

I was wrong last round about the barrels being equivalent to re-exporting from types.ts - sorry. runtime/index.ts re-exports logging, analytics and auth with explicit named lists rather than export * (only ./slots gets export *, which is why SlotOperation reaches consumers). So export type * from './types' dead-ends here, and import { LoggingService } from '@openedx/frontend-base' fails with TS2724. The *ServiceClass types do get through, via types.ts.

arbrandes · 2026-09-01T15:51:17Z


  // Analytics
-  segmentKey: string | null;
+  segmentKey: string | null,


Run npm run lint:fix. The new members use commas where the repo uses semicolons - 27 @stylistic/member-delimiter-style warnings across types.ts, runtime/auth/types.ts and runtime/analytics/types.ts. This line also flips the pre-existing segmentKey: string | null; to a comma.

arbrandes · 2026-09-01T15:51:17Z

+import { LoggingService } from './runtime/logging';
+import { AnalyticsService } from './runtime/analytics';
+import { AuthService } from './runtime/auth';


Minor: import from ./runtime/logging/types etc. rather than the value barrels, matching the SlotOperation import on line 4. As written it forms a cycle (types.ts -> runtime/auth/index.ts -> runtime/auth/types.ts -> types.ts); harmless since tsc elides these, but avoidable.

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

Comment thread types.ts Outdated

Comment thread types.ts Outdated

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

vkumar-sonata added 2 commits August 31, 2026 05:37

fix: refine service constructor types based on reviewer feedback

8c5c1a8

fix: resolve conflicts when upstream from opensource main branch

2f5cb31

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

arbrandes requested changes Sep 1, 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!

Uh oh!

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!

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!

vkumar-sonata Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vkumar-sonata commented Aug 31, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vkumar-sonata commented Jul 21, 2026 •

edited by arbrandes

Loading

vkumar-sonata Aug 31, 2026 •

edited

Loading