feat: Convert project config manager module to TS #684
Conversation
yavorona
force-pushed
the
pnguen/convert-project-config-manager
branch
from
f5f6ccf to
c201a8f
Compare
yavorona
force-pushed
the
pnguen/convert-project-config-manager
branch
from
55dc6be to
b986680
Compare
| import { toDatafile, tryCreatingProjectConfig } from '../../core/project_config'; | ||
| import { createOptimizelyConfig } from '../optimizely_config'; | ||
| import { OptimizelyConfig, DatafileOptions, DatafileManagerConfig } from '../../shared_types'; | ||
| import { ProjectConfig } from '../project_config'; |
There was a problem hiding this comment.
This and line 22 can be imported in one import because i think they are referring to the same package using different paths
| this.configObj = null; | ||
| this.optimizelyConfigObj = null; | ||
| this.updateListeners = []; | ||
| this.jsonSchemaValidator = config.jsonSchemaValidator; | ||
| this.datafileManager = null; |
There was a problem hiding this comment.
Is there a specific reason to define these separately in constructors. Can these initialization values go with instance variable declarations above the constructor?
There was a problem hiding this comment.
That is a great idea :)
| logger.error(ex); | ||
| this.updateListeners = []; | ||
| this.configObj = null; | ||
| this.optimizelyConfigObj = null; | ||
| this.datafileManager = null; | ||
| this.readyPromise = Promise.resolve({ | ||
| success: false, | ||
| reason: getErrorMessage(ex, 'Error in initialize'), | ||
| }); |
There was a problem hiding this comment.
Some of these might not be needed here if you initialize default values with instance variable declarations at the top of the class.
| public datafileManager: HttpPollingDatafileManager | null; | ||
|
|
||
| constructor(config: ProjectConfigManagerConfig) { | ||
| try { |
There was a problem hiding this comment.
Why did you see a need to add a try block here when it wasnt there in the JS version?
There was a problem hiding this comment.
It was in the original implementation here:
https://github.com/optimizely/javascript-sdk/pull/684/files#diff-8c34af3f23d5d0c9fa26e08c7ea42c8d71342761c3ac3d7669fff4e3c6127027L55
| * successful result. | ||
| */ | ||
| private onDatafileManagerReadyFulfill(): { success: boolean; reason?: string } { | ||
| if (this.datafileManager) { |
There was a problem hiding this comment.
Why did you add this extra check in the typescript version. datafile manager is not expected to be null here because promise can only be fulfilled after its initialization
There was a problem hiding this comment.
True but TS compiler is not smart enough to know it. It requires this check prior to calling this.datafileManager.get() since the initial value of it is null.
| private updateListeners: Array<(config: ProjectConfig) => void>; | ||
| private configObj: ProjectConfig | null; | ||
| private optimizelyConfigObj: OptimizelyConfig | null; | ||
| private readyPromise: Promise<{ success: boolean; reason?: string }>; |
There was a problem hiding this comment.
Suggestion (Non binding): I see you using { success: boolean; reason?: string } type at many places in the file. What do you think about declaring a type at the top and reusing it for simplicity. Totally up to your preference.
| * update listeners if successful | ||
| */ | ||
| private onDatafileManagerUpdate(): void { | ||
| if (this.datafileManager) { |
There was a problem hiding this comment.
Again, why do you need a check here?
| this.updateListeners.forEach((listener) => { | ||
| listener(configObj); | ||
| }); |
There was a problem hiding this comment.
| this.updateListeners.forEach((listener) => { | |
| listener(configObj); | |
| }); | |
| this.updateListeners.forEach((listener) => listener(configObj)); |
| declare module '@optimizely/js-sdk-datafile-manager' { | ||
| interface DatafileManagerConfig { | ||
| sdkKey: string; | ||
| } | ||
| interface DatafileUpdate { | ||
| datafile: string; | ||
| } | ||
| type Disposer = () => void; | ||
|
|
||
| export class HttpPollingDatafileManager { | ||
| constructor(config: DatafileManagerConfig); | ||
| start(): void; | ||
| onReady(): Promise<void>; | ||
| on(eventName: string, listener: (datafileUpdate: DatafileUpdate) => void): Disposer; | ||
| get(): string; | ||
| stop(): Promise<void>; | ||
| } | ||
| } |
There was a problem hiding this comment.
The indentation looks off. it should be 2 spaces per tab.
Summary
project_config_manager.jsto TSTest plan
Existing unit tests