Skip to content
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

[FSSDK-10882] ProjectConfigManager SSR support #965

Merged
Merged
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: 0 additions & 1 deletion lib/index.node.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ describe('optimizelyFactory', function() {
assert.instanceOf(optlyInstance, Optimizely);
assert.equal(optlyInstance.clientVersion, '5.3.4');
});

// TODO: user will create and inject an event processor
// these tests will be refactored accordingly
// describe('event processor configuration', function() {
Expand Down
78 changes: 78 additions & 0 deletions lib/optimizely/index.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/**
* Copyright 2024, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { describe, it, expect, vi } from 'vitest';
import Optimizely from '.';
import { getMockProjectConfigManager } from '../tests/mock/mock_project_config_manager';
import * as logger from '../plugins/logger';
import * as jsonSchemaValidator from '../utils/json_schema_validator';
import { LOG_LEVEL } from '../common_exports';
import { createNotificationCenter } from '../core/notification_center';
import testData from '../tests/test_data';
import { getForwardingEventProcessor } from '../event_processor/forwarding_event_processor';
import { LoggerFacade } from '../modules/logging';
import { createProjectConfig } from '../project_config/project_config';

describe('lib/optimizely', () => {
const errorHandler = { handleError: function() {} };

const eventDispatcher = {
dispatchEvent: () => Promise.resolve({ statusCode: 200 }),
};

const eventProcessor = getForwardingEventProcessor(eventDispatcher);

const createdLogger: LoggerFacade = {
...logger.createLogger({
logLevel: LOG_LEVEL.INFO,
}),
info: () => {},
debug: () => {},
warn: () => {},
error: () => {},
log: () => {},
};

const notificationCenter = createNotificationCenter({ logger: createdLogger, errorHandler });

it('should pass ssr to the project config manager', () => {
const projectConfigManager = getMockProjectConfigManager({
initConfig: createProjectConfig(testData.getTestProjectConfig()),
});

vi.spyOn(projectConfigManager, 'setSsr');

const instance = new Optimizely({
clientEngine: 'node-sdk',
projectConfigManager,
errorHandler,
jsonSchemaValidator,
logger: createdLogger,
notificationCenter,
eventProcessor,
isSsr: true,
isValidInstance: true,
});

expect(projectConfigManager.setSsr).toHaveBeenCalledWith(true);
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
expect(instance.getProjectConfig()).toBe(projectConfigManager.config);
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
expect(projectConfigManager.isSsr).toBe(true);
});
});
1 change: 1 addition & 0 deletions lib/optimizely/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ export default class Optimizely implements Client {
this.updateOdpSettings();
});

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

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

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
21 changes: 21 additions & 0 deletions lib/project_config/project_config_manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,17 @@ describe('ProjectConfigManagerImpl', () => {
await manager.onRunning();
expect(manager.getConfig()).toEqual(createProjectConfig(testData.getTestProjectConfig()));
});

it('should not start datafileManager if isSsr is true and return correct config', () => {
const datafileManager = getMockDatafileManager({});
vi.spyOn(datafileManager, 'start');
const manager = new ProjectConfigManagerImpl({ datafile: testData.getTestProjectConfig(), datafileManager });
manager.setSsr(true);
manager.start();

expect(manager.getConfig()).toEqual(createProjectConfig(testData.getTestProjectConfig()));
expect(datafileManager.start).not.toHaveBeenCalled();
});
});

describe('when datafile is invalid', () => {
Expand Down Expand Up @@ -398,6 +409,16 @@ describe('ProjectConfigManagerImpl', () => {
expect(logger.error).toHaveBeenCalled();
});

it('should reject onRunning() and log error if isSsr is true and datafile is not provided', async () =>{
const logger = getMockLogger();
const manager = new ProjectConfigManagerImpl({ logger, datafileManager: getMockDatafileManager({})});
manager.setSsr(true);
manager.start();

await expect(manager.onRunning()).rejects.toThrow();
expect(logger.error).toHaveBeenCalled();
});

it('should reject onRunning() and log error if the datafile version is not supported', async () => {
const logger = getMockLogger();
const datafile = testData.getUnsupportedVersionConfig();
Expand Down
22 changes: 21 additions & 1 deletion lib/project_config/project_config_manager.ts
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 @@ -53,6 +54,7 @@ export class ProjectConfigManagerImpl extends BaseService implements ProjectConf
public jsonSchemaValidator?: Transformer<unknown, boolean>;
public datafileManager?: DatafileManager;
private eventEmitter: EventEmitter<{ update: ProjectConfig }> = new EventEmitter();
private isSsr = false;

constructor(config: ProjectConfigManagerConfig) {
super();
Expand All @@ -68,9 +70,18 @@ export class ProjectConfigManagerImpl extends BaseService implements ProjectConf
}

this.state = ServiceState.Starting;

if(this.isSsr) {
Copy link
Contributor

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

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

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.datafileManager) {
const errorMessage = this.isSsr
? 'You must provide datafile in SSR'
: 'You must provide at least one of sdkKey or datafile';
// TODO: replace message with imported constants
this.handleInitError(new Error('You must provide at least one of sdkKey or datafile'));
this.handleInitError(new Error(errorMessage));
return;
}

Expand Down Expand Up @@ -211,4 +222,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 {
this.isSsr = isSsr;
}
}
2 changes: 2 additions & 0 deletions lib/shared_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ export interface OptimizelyOptions {
sdkKey?: string;
userProfileService?: UserProfileService | null;
defaultDecideOptions?: OptimizelyDecideOption[];
isSsr?:boolean;
odpManager?: IOdpManager;
notificationCenter: NotificationCenterImpl;
}
Expand Down Expand Up @@ -426,6 +427,7 @@ export interface ConfigLite {
defaultDecideOptions?: OptimizelyDecideOption[];
clientEngine?: string;
clientVersion?: string;
isSsr?: boolean;
}

export type OptimizelyExperimentsMap = {
Expand Down
4 changes: 4 additions & 0 deletions lib/tests/mock/mock_project_config_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,12 @@ type MockOpt = {

export const getMockProjectConfigManager = (opt: MockOpt = {}): ProjectConfigManager => {
return {
isSsr: false,
config: opt.initConfig,
start: () => {},
setSsr: function(isSsr:boolean) {
this.isSsr = isSsr;
},
onRunning: () => opt.onRunning || Promise.resolve(),
stop: () => {},
onTerminated: () => opt.onTerminated || Promise.resolve(),
Expand Down
Loading