[FSSDK-10882] ProjectConfigManager SSR support #965
Conversation
| @@ -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.
instead of optional boolean, we can use a boolean with default false
private isSsr = false;
| * @param {Boolean} isSsr | ||
| * @returns {void} | ||
| */ | ||
| setSsr(isSsr?: boolean): void { |
There was a problem hiding this comment.
can we use boolean parameter instead of optional boolean?
| @@ -79,6 +81,11 @@ export class ProjectConfigManagerImpl extends BaseService implements ProjectConf | |||
| return; | |||
| } | |||
|
|
|||
| if(this.isSsr) { | |||
There was a problem hiding this comment.
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.
Also can we add a test that if isSsr is provided without a datafile, onRunning() is rejected?
There was a problem hiding this comment.
Good point. I have missed that edge case. Thanks for pointing it out..
| @@ -146,6 +146,7 @@ export default class Optimizely implements Client { | |||
| this.updateOdpSettings(); | |||
| }); | |||
|
|
|||
| this.projectConfigManager.setSsr(config.isSsr) | |||
There was a problem hiding this comment.
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.
Ok. I did not know about that change.
lib/index.node.tests.js
Outdated
| @@ -88,6 +89,22 @@ describe('optimizelyFactory', function() { | |||
| assert.equal(optlyInstance.clientVersion, '5.3.4'); | |||
| }); | |||
|
|
|||
| it('should create an instance of optimizely with ssr flag, project config must be available', () => { | |||
There was a problem hiding this comment.
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.
This won't be required anymore due to the new index.spec.ts coverage
junaed-optimizely
deleted the
junaed/fssdk-10882-project-config-ssr-support
branch
Summary
This PR makes the
ProjectConfigManagerSSR compatibleTest plan
Issues