-
Notifications
You must be signed in to change notification settings - Fork 83
[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.
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
Changes from 2 commits
9667701
e1bdc8b
c4c98a1
6873375
3777312
9b0ff64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,6 +146,7 @@ export default class Optimizely implements Client { | |
this.updateOdpSettings(); | ||
}); | ||
|
||
this.projectConfigManager.setSsr(config.isSsr) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
@@ -79,6 +81,11 @@ export class ProjectConfigManagerImpl extends BaseService implements ProjectConf | |
return; | ||
} | ||
|
||
if(this.isSsr) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we use boolean parameter instead of optional boolean? |
||
this.isSsr = isSsr; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add the test in a new index.node.spec.ts file instead? We want to get rid of all JS tests. We will only update existing tests in these old tests file and will add all new tests in new ts test files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be required anymore due to the new
index.spec.ts
coverage