fix: React SDK null client handling #149

opti-jnguyen · 2022-02-11T19:10:40Z

Summary

Handle React SDK fail catching if ._client returns null when creating new ReactClientSDK instance from createInstance function.

Test Plan

Manually tested thoroughly.
All existing unit tests pass.

Issues

#134

Merge changes from main branch

zashraf1985

Great Work! Its looking really good. I have requested a few minor changes. After that, this one is good to go!

zashraf1985 · 2022-02-25T02:10:14Z

src/client.ts

 export type OnReadyResult = {
  success: boolean;
-  reason?: string;
+  reason?: NotReadyReason;
+  reasonDetail?: string;


A Non binding suggestion: Does it make sense to call it message instead of reasonDetail? This will make the success case look more inuitive where we dont provide a reason but only reasonDetail right now. But its just a personal preference.. Totally non binding.

zashraf1985 · 2022-02-25T02:19:42Z

src/client.ts

      return;
    }

-    return this._client.track(eventKey, user.id, user.attributes, eventTags);
+    this._client.track(eventKey, user.id, user.attributes, eventTags);


Good catch! This was an unnecessary return 👍

zashraf1985 · 2022-02-25T02:20:57Z

src/client.ts

@@ -581,7 +651,7 @@ class OptimizelyReactSDKClient implements ReactSDKClient {
   */
  public removeAllForcedDecisions(): boolean {
    if (!this.userContext) {
-      logger.info("Can't remove a forced decision because the user context has not been set yet");
+      logger.warn('Unable to remove a forced decision because the user context has not been set yet.');


Suggested change

logger.warn('Unable to remove a forced decision because the user context has not been set yet.');

logger.warn('Unable to remove forced decisions because the user context has not been set yet.');

zashraf1985 · 2022-02-25T02:22:14Z

src/client.ts

    const user = this.getUserContextWithOverrides(overrideUserId, overrideAttributes);
    if (user.id === null) {
-      logger.info('isFeatureEnabled returning false for feature "%s" because userId is not set', feature);
+      logger.warn('Unable to determine if feature "%s" is enabled because User ID is not set', feature);


Log message nicely rewritten 👍

zashraf1985 · 2022-02-25T02:24:02Z

src/client.ts

    const feature = optlyConfig.featuresMap[featureKey];
    if (!feature) {
+      logger.warn(


I would suggest to convert that to info. Its probably not an unusual behavior.

zashraf1985 · 2022-02-25T02:27:29Z

src/client.ts

    if (user.id === null) {
+      logger.warn('Unable to get feature variable boolean from feature "%s" because User ID is not set', feature);


Convert to info

zashraf1985 · 2022-02-25T02:27:45Z

src/client.ts

    if (user.id === null) {
+      logger.warn('Unable to get feature variable integer from feature "%s" because User ID is not set', feature);


make it info

zashraf1985 · 2022-02-25T02:30:26Z

src/client.ts

@@ -887,24 +1096,66 @@ class OptimizelyReactSDKClient implements ReactSDKClient {
   *  @returns {optimizely.OptimizelyConfig | null} optimizely config
   */
  public getOptimizelyConfig(): optimizely.OptimizelyConfig | null {
+    if (!this._client) {
+      logger.warn('Unable to get Optimizely configuration because Optimizely client failed to initialize.');


Lets call it config. We never use the full term configuration throughout our ecosystem

zashraf1985 · 2022-02-25T02:33:55Z

src/hooks.ts

+      switch (res.reason) {
+        // Optimizely client failed to initialize.
+        case 'NO_CLIENT':
+          hooksLogger.info(`Client not ready, reason="${res.reasonDetail}"`);


We can probably convert this one to warn. what do you think?

zashraf1985 · 2022-02-25T02:35:32Z

src/hooks.ts

@@ -431,5 +452,6 @@ export const useDecision: UseDecision = (flagKey, options = {}, overrides = {})
    return (): void => {};
  }, [optimizely.getIsReadyPromiseFulfilled(), options.autoUpdate, optimizely, flagKey, getCurrentDecision]);

+  // CHECK: Return clientReady as false & didTimeout as true if ._client is null.


This was probably your own note that you forgot to delete?

Develliot · 2022-02-28T13:14:28Z

would it also be to have the Provider silently fails if the optimizely prop (ReactSDKClient) is null when createInstance() fails or isn't possible at time of render, to make it less brittle.

I believe It's got an update handler to handle optimizely prop being added later, it would be nice if the useDecision in stead of throwing an error without a ReactSDKClient so it returned {data, error, isLoading}.

zashraf1985

LGTM! Great Work.

opti-jnguyen · 2022-02-28T17:48:18Z

would it also be to have the Provider silently fails if the optimizely prop (ReactSDKClient) is null when createInstance() fails or isn't possible at time of render, to make it less brittle.

I believe It's got an update handler to handle optimizely prop being added later, it would be nice if the useDecision in stead of throwing an error without a ReactSDKClient so it returned {data, error, isLoading}.

@Develliot - thanks for the suggestion! We discussed this approach internally and due to the nature of the change, we will consider the change as a new feature request as it requires refactoring many components.

Our suggestion is to add it to our feature request board here - we will prioritize it based on the number of votes it gets!

Best,
John

fabb · 2022-02-28T20:09:05Z

src/client.ts

+    } else {
+      logger.warn('Unable to resolve datafile and user information because Optimizely client failed to initialize.');
+
+      this.dataReadyPromise = new Promise((resolve, reject) => {


You could use Promise.resolve(value)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/resolve

Thanks @fabb - we'll implement that change in a future push!

opti-jnguyen added 2 commits January 31, 2022 14:59

WIP: Client Null Handling

9c9140e

WIP: Client Null Handling

efd9bc7

opti-jnguyen requested a review from zashraf1985 February 11, 2022 19:10

optimizely-codereview-bot assigned opti-jnguyen Feb 11, 2022

opti-jnguyen changed the title ~~Jnguyen/client null handling~~ fix: React SDK null client handling Feb 11, 2022

opti-jnguyen marked this pull request as draft February 11, 2022 19:12

opti-jnguyen added 6 commits February 11, 2022 14:36

Merge branch 'master' into jnguyen/client-null-handling

94ae76b

WIP: Client Null Handling

03178c5

WIP: Revert User ID changes

e21a580

WIP: Client Null Handling

9cba7ce

Finish Client Null Handling + Client Unit Tests

770069e

Merge branch 'master' into jnguyen/client-null-handling

5a85ce5

Merge changes from main branch

opti-jnguyen marked this pull request as ready for review February 24, 2022 21:03

zashraf1985 suggested changes Feb 25, 2022

View reviewed changes

Client Null Handling PR Review Revisions

ff1f649

zashraf1985 approved these changes Feb 28, 2022

View reviewed changes

Updated Changelog for PR 149

dd8f7f8

opti-jnguyen merged commit 1663d01 into master Feb 28, 2022

opti-jnguyen deleted the jnguyen/client-null-handling branch February 28, 2022 17:45

opti-jnguyen mentioned this pull request Feb 28, 2022

createInstance can throw: how to graceful fallback #134

Closed

fabb reviewed Feb 28, 2022

View reviewed changes

opti-jnguyen restored the jnguyen/client-null-handling branch March 7, 2022 19:28

opti-jnguyen deleted the jnguyen/client-null-handling branch March 7, 2022 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: React SDK null client handling #149

fix: React SDK null client handling #149

opti-jnguyen commented Feb 11, 2022 •

edited

Loading

zashraf1985 left a comment

zashraf1985 Feb 25, 2022

zashraf1985 Feb 25, 2022

zashraf1985 Feb 25, 2022

zashraf1985 Feb 25, 2022

zashraf1985 Feb 25, 2022

zashraf1985 Feb 25, 2022

zashraf1985 Feb 25, 2022

zashraf1985 Feb 25, 2022

zashraf1985 Feb 25, 2022

zashraf1985 Feb 25, 2022

Develliot commented Feb 28, 2022 •

edited

Loading

zashraf1985 left a comment

opti-jnguyen commented Feb 28, 2022

fabb Feb 28, 2022

opti-jnguyen Mar 2, 2022

	logger.warn('Unable to remove a forced decision because the user context has not been set yet.');
	logger.warn('Unable to remove forced decisions because the user context has not been set yet.');

		if (user.id === null) {
		logger.warn('Unable to get feature variable boolean from feature "%s" because User ID is not set', feature);

fix: React SDK null client handling #149

fix: React SDK null client handling #149

Conversation

opti-jnguyen commented Feb 11, 2022 • edited Loading

Summary

Test Plan

Issues

zashraf1985 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Develliot commented Feb 28, 2022 • edited Loading

zashraf1985 left a comment

Choose a reason for hiding this comment

opti-jnguyen commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opti-jnguyen commented Feb 11, 2022 •

edited

Loading

Develliot commented Feb 28, 2022 •

edited

Loading