Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ module.exports = tseslint.config(
'test-site/*',
'config/*',
'docs/*',
'test-types/*',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

],
},
);
7 changes: 7 additions & 0 deletions runtime/analytics/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export interface AnalyticsService {
sendTrackingLogEvent(eventName: string, properties: object): Promise<void>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

identifyAuthenticatedUser(userId: string | number, traits?: Record<string, unknown>): void,
identifyAnonymousUser(traits?: Record<string, unknown>): void,
sendTrackEvent(eventName?: string, properties?: Record<string, unknown>): void,
sendPageEvent(category: string, name: string, properties?: Record<string, unknown>): void,
}
13 changes: 13 additions & 0 deletions runtime/auth/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export interface AuthService {
getAuthenticatedHttpClient(options?: Record<string, unknown>): unknown,
getHttpClient(options?: Record<string, unknown>): unknown,
getLoginRedirectUrl(redirectUrl?: string): string,
redirectToLogin(redirectUrl?: string): void,
getLogoutRedirectUrl(redirectUrl?: string): string,
redirectToLogout(redirectUrl?: string): void,
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>>,
Comment on lines +8 to +11

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

hydrateAuthenticatedUser(): Promise<null>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
18 changes: 18 additions & 0 deletions test-types/site-config-service-overrides.typecheck.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
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;
Comment on lines +1 to +18

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

37 changes: 37 additions & 0 deletions types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ import { FC, ReactElement, ReactNode } from 'react';
import { MessageDescriptor } from 'react-intl';
import { RouteObject } from 'react-router';
import { SlotOperation } from './runtime/slots/types';
import { LoggingService } from './runtime/logging/types';
import { AnalyticsService } from './runtime/analytics/types';
import { AuthService } from './runtime/auth/types';

// Apps

Expand Down Expand Up @@ -59,6 +62,35 @@ export interface RequiredSiteConfig {
export type LocalizedMessages = Record<string, Record<string, string>>;
export type SiteMessages = LocalizedMessages[];

export type { LoggingService, AnalyticsService, AuthService };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Logging instantiated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

export type LoggingServiceClass = new (options: {
config: SiteConfig,
}) => LoggingService;

// Analytics instantiated
export type AnalyticsServiceClass = new (options: {
config: SiteConfig,
loggingService: LoggingService,
httpClient: unknown,
}) => AnalyticsService;

// Auth instantiated
export type AuthServiceClass = new (options: {
config: {
baseUrl: string,
lmsBaseUrl: string,
loginUrl: string,
logoutUrl: string,
refreshAccessTokenApiPath: string,
accessTokenCookieName: string,
csrfTokenApiPath: string,
},
loggingService: object,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

middleware?: unknown[],
Comment on lines +81 to +91

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}) => AuthService;

export interface OptionalSiteConfig {
// Site environment
environment: EnvironmentTypes,
Expand Down Expand Up @@ -92,6 +124,11 @@ export interface OptionalSiteConfig {

// Analytics
segmentKey: string | null,

// Services
loggingService: LoggingServiceClass,
analyticsService: AnalyticsServiceClass,
authService: AuthServiceClass,
}

export type SiteConfig = RequiredSiteConfig & Partial<OptionalSiteConfig>;
Expand Down