Skip to content

[FSSDK-10882] ProjectConfigManager SSR support #965

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 22, 2024
Merged

[FSSDK-10882] ProjectConfigManager SSR support #965

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9667701
[FSSDK-10882] ssr support addition
junaed-optimizely Nov 18, 2024
e1bdc8b
[FSSDK-10882] test addition
junaed-optimizely Nov 20, 2024
c4c98a1
[FSSDK-10882] improvements
junaed-optimizely Nov 21, 2024
6873375
[FSSDK-10882] improvements
junaed-optimizely Nov 21, 2024
3777312
[FSSDK-10882] fix
junaed-optimizely Nov 21, 2024
9b0ff64
Merge branch 'master' into junaed/fssdk-10882-project-config-ssr-support
junaed-optimizely Nov 22, 2024
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
Next Next commit
[FSSDK-10882] ssr support addition
  • Loading branch information
@junaed-optimizely
junaed-optimizely committed Nov 18, 2024
commit 9667701d0a3ef899d456cd9b51a416081168c9e4
1 change: 1 addition & 0 deletions lib/optimizely/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ export default class Optimizely implements Client {
this.updateOdpSettings();
});

this.projectConfigManager.setSsr(config.isSsr)
Copy link
Contributor

@raju-opti raju-opti Nov 20, 2024

Choose a reason for hiding this comment

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

can we add a test that isSsr is passed to the projectConfigManager?

Please do this in a new lib/optimizely/index.spec.ts file instead of the old js test file. We will eventually get rid of the whole js test file.

Copy link
Contributor Author

@junaed-optimizely junaed-optimizely Nov 20, 2024

Choose a reason for hiding this comment

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

Ok. I did not know about that change.

this.projectConfigManager.start();
const projectConfigManagerRunningPromise = this.projectConfigManager.onRunning();

Expand Down
16 changes: 16 additions & 0 deletions lib/project_config/project_config_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ interface ProjectConfigManagerConfig {

export interface ProjectConfigManager extends Service {
setLogger(logger: LoggerFacade): void;
setSsr(isSsr?: boolean): void;
getConfig(): ProjectConfig | undefined;
getOptimizelyConfig(): OptimizelyConfig | undefined;
onUpdate(listener: Consumer<ProjectConfig>): Fn;
Expand All @@ -54,6 +55,7 @@ export class ProjectConfigManagerImpl extends BaseService implements ProjectConf
public datafileManager?: DatafileManager;
private eventEmitter: EventEmitter<{ update: ProjectConfig }> = new EventEmitter();
private logger?: LoggerFacade;
private isSsr?: boolean;
Copy link
Contributor

@raju-opti raju-opti Nov 20, 2024

Choose a reason for hiding this comment

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

instead of optional boolean, we can use a boolean with default false

private isSsr = false;


constructor(config: ProjectConfigManagerConfig) {
super();
Expand All @@ -79,6 +81,11 @@ export class ProjectConfigManagerImpl extends BaseService implements ProjectConf
return;
}

if(this.isSsr) {
Copy link
Contributor

@raju-opti raju-opti Nov 20, 2024

Choose a reason for hiding this comment

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

if isSsr is true no datafile is provided, but only a datafileManager is provided, then onRunning() will wait indefinitely cause we are dropping the datafile. We can move this condition before the previous if on line 78 and update the message inside for isSsr case

Copy link
Contributor

@raju-opti raju-opti Nov 20, 2024

Choose a reason for hiding this comment

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

Also can we add a test that if isSsr is provided without a datafile, onRunning() is rejected?

Copy link
Contributor Author

@junaed-optimizely junaed-optimizely Nov 20, 2024

Choose a reason for hiding this comment

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

Good point. I have missed that edge case. Thanks for pointing it out..

// If isSsr is true, we don't need to poll for datafile updates
this.datafileManager = undefined
}

if (this.datafile) {
this.handleNewDatafile(this.datafile, true);
}
Expand Down Expand Up @@ -216,4 +223,13 @@ export class ProjectConfigManagerImpl extends BaseService implements ProjectConf
this.stopPromise.reject(err);
});
}

/**
* Set the isSsr flag to indicate if the project config manager is being used in a server side rendering environment
* @param {Boolean} isSsr
* @returns {void}
*/
setSsr(isSsr?: boolean): void {
Copy link
Contributor

@raju-opti raju-opti Nov 20, 2024

Choose a reason for hiding this comment

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

can we use boolean parameter instead of optional boolean?

this.isSsr = isSsr;
}
}
1 change: 1 addition & 0 deletions lib/shared_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ export interface OptimizelyOptions {
sdkKey?: string;
userProfileService?: UserProfileService | null;
defaultDecideOptions?: OptimizelyDecideOption[];
isSsr?:boolean;
odpManager?: IOdpManager;
notificationCenter: NotificationCenterImpl;
}
Expand Down
1 change: 1 addition & 0 deletions lib/tests/mock/mock_project_config_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const getMockProjectConfigManager = (opt: MockOpt = {}): ProjectConfigMan
return {
config: opt.initConfig,
start: () => {},
setSsr: () => {},
onRunning: () => opt.onRunning || Promise.resolve(),
stop: () => {},
onTerminated: () => opt.onTerminated || Promise.resolve(),
Expand Down
Loading