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.

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

Jump to bottom

Add Notification Registry for ODP Setting Updates #795

Merged
merged 6 commits into from
Feb 7, 2023
Merged

Add Notification Registry for ODP Setting Updates #795

merged 6 commits into from
Feb 7, 2023

Conversation

opti-jnguyen
Copy link
Contributor

@opti-jnguyen opti-jnguyen commented Jan 27, 2023 •
edited
Loading

Summary

(Copied from corresponding Java PR: optimizely/java-sdk#501)

  • NotificationRegistry is added to create default NotificationCenter for internal use for updating ODPConfig.
  • This is to handle a corner case that if in any case user passes different notificationCenter to Optimizely and ProjectConfigManager then the Notification of UpdateProjectConfig will not trigger because its handler is getting added in the main NotificationCenter object.
  • This will make sure UpdateProjectConfig will update ODPConfig every time irrespective of different NotificationCenter passed by user.

Note: cachedConfig doesn't appear to be needed for JS implementation as getConfig() already returns a locally stored configObject: ProjectConfig in memory without the need to await for a return

Test plan

  • Added additional unit test to test NotificationRegistry
  • All tests should pass.

Issues

@opti-jnguyen opti-jnguyen requested a review from a team as a code owner January 27, 2023 13:52
mikechu-optimizely
mikechu-optimizely reviewed Jan 27, 2023
View reviewed changes
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

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

Just some thoughts. I'm not sure I'm the official code reviewer so....

packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts Show resolved Hide resolved
packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/core/odp/odp_config.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/core/odp/odp_config.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/core/odp/odp_config.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/shared_types.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/tests/odpManager.spec.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/tests/odpSegmentApiManager.ts Outdated Show resolved Hide resolved
jaeopt
jaeopt requested changes Jan 27, 2023
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

A suggestion to clean up redundant sdkKey in 2 places.

packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/core/odp/odp_manager.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/core/odp/odp_manager.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/optimizely/index.ts Outdated
if (this.projectConfigManager.getConfig() != null) {
this.updateODPSettings();
}
const sdkKey = this.projectConfigManager.getConfig()?.sdkKey;
Copy link
Contributor

@jaeopt jaeopt Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We have sdkKey here (not indirectly from projectConfigManager) in line 132.
We can consider remove "getSdkKey()" new method from projectConfigManager and use a local value.
Also, we have to change "sdkKey" as mandatory (not null), which may be a breaking change in the release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch - swapped out .getConfig()?sdkKey references with local config.sdkKey usage.

Regarding changing sdkKey to be mandatory, this will definitely be a breaking change - especially since there's some nuances (for example, the Lite bundle does not expect sdkKey to begin with). Brainstorming on this one.

packages/optimizely-sdk/lib/optimizely/index.ts Outdated Show resolved Hide resolved
msohailhussain
msohailhussain reviewed Feb 2, 2023
View reviewed changes
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

have some questions, we may discuss offline.

packages/optimizely-sdk/lib/modules/datafile-manager/httpPollingDatafileManager.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/optimizely/index.ts Outdated
@@ -175,6 +179,27 @@ export default class Optimizely {

this.readyTimeouts = {};
this.nextReadyTimeoutId = 0;

if (config.odpManager != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

All this logic should come-up after the readyPromise? How config.sdkKey will be non-null if the datafile is still being fetched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving logic into Promise.all.

Also, as discussed, this should catch for both cases where a new Optimizely instance is provided either an SDK Key or manually includes a datafile.

packages/optimizely-sdk/lib/optimizely/index.ts Outdated Show resolved Hide resolved
jaeopt
jaeopt requested changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

A few changes suggested.

packages/optimizely-sdk/lib/core/odp/odp_config.ts Outdated Show resolved Hide resolved
packages/optimizely-sdk/lib/core/project_config/index.ts Show resolved Hide resolved
packages/optimizely-sdk/lib/core/project_config/index.tests.js Show resolved Hide resolved
packages/optimizely-sdk/lib/optimizely/index.ts Outdated
if (this.projectConfigManager.getConfig() != null) {
this.updateODPSettings();
}
if (config.sdkKey != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have 2 sources of sdkKey, here and projectConfigManager. It looks like sdkKey is optional here (when datafile is provided and no polling required). In projectConfigManager can return a valid sdkKey in either case.
So, it may be a better to get sdkKey consistently from projectConfigManager. In this way, we do not see any breaking changes required.

packages/optimizely-sdk/lib/optimizely/index.ts Show resolved Hide resolved
jaeopt
jaeopt approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

@opti-jnguyen opti-jnguyen merged commit f6d71e1 into jnguyen/odp-manager Feb 7, 2023
@opti-jnguyen opti-jnguyen deleted the jnguyen/fix-notification-center-issue branch February 7, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikechu-optimizely mikechu-optimizely mikechu-optimizely left review comments

@msohailhussain msohailhussain msohailhussain left review comments

@jaeopt jaeopt jaeopt approved these changes

Assignees

@opti-jnguyen opti-jnguyen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@opti-jnguyen @msohailhussain @jaeopt @mikechu-optimizely