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!

… fix linting issues

vkumar-sonata · 2026-09-02T06:23:19Z

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

@arbrandes Acknowledged and made the changes as per the feedback. Please review the changes.

arbrandes

Still a couple of issues.

…ignature

vkumar-sonata · 2026-09-03T06:07:44Z

Acknowledged and made the changes as per the feedback. Please review the changes.

@arbrandes Acknowledged. Made the fixes as per the feedback. Please review the changes.

arbrandes

Thanks again. One last thing and I'll merge it.

arbrandes · 2026-09-03T14:20:55Z

-   * @returns {Promise<null>}
+   * @returns {Promise<void>}
   */
  hydrateAuthenticatedUser = jest.fn(() => {


authService: MockAuthService still doesn't compile - the async half of the last round's fix didn't make it in. Change this to jest.fn(async () => {.

Fixed. The async keyword was missed in the previous commit, hydrateAuthenticatedUser is now jest.fn(async () => {, matching the Promise<void> return type.

vkumar-sonata · 2026-09-04T11:22:18Z

Thanks again. One last thing and I'll merge it.

@arbrandes Apologies. I am not sure how I missed that. This time I have addressed that. Please kindly review the changes. Thanks.

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

Comment thread types.ts

Comment thread runtime/auth/types.ts Outdated

Comment thread runtime/auth/index.ts Outdated

Comment thread types.ts Outdated

Comment thread types.ts Outdated

fix: resolve AxiosJwtAuthService JSDocs types, fix barrel exports and…

f817779

… fix linting issues

vkumar-sonata requested a review from arbrandes September 2, 2026 06:25

arbrandes requested changes Sep 2, 2026

View reviewed changes

Comment thread types.ts

Comment thread runtime/analytics/types.ts Outdated

Comment thread runtime/analytics/types.ts Outdated

fix: apply MockAuthService JSDoc fixes and correct analytics method s…

b3bf73b

…ignature

vkumar-sonata requested a review from arbrandes September 3, 2026 06:07

arbrandes requested changes Sep 3, 2026

View reviewed changes

fix: make hydrateAuthenticatedUser async in MockAuthService

abd3732

vkumar-sonata requested a review from arbrandes September 4, 2026 11:22

		@@ -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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vkumar-sonata commented Sep 2, 2026

Uh oh!

arbrandes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vkumar-sonata commented Sep 3, 2026

Uh oh!

arbrandes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vkumar-sonata commented Jul 21, 2026 •

edited by arbrandes

Loading

vkumar-sonata Aug 31, 2026 •

edited

Loading