converted entry level modules to ts #590

ozayr-zaviar · 2020-10-06T05:53:06Z

Summary

Converted packages\optimizely-sdk\lib\index.node.js to TS
Converted packages\optimizely-sdk\lib\index.browser.js to TS
Converted packages\optimizely-sdk\lib\index.react_native.js to TS

Test plan

All existing unit tests should pass
All existing test cases on FSC should pass

coveralls · 2020-10-06T05:57:50Z

Coverage increased (+0.03%) to 96.662% when pulling e85f578 on uzair/entry_level_modules_to_ts into 0576e46 on master.

msohailhussain

Please address and fix all issues.

msohailhussain · 2020-10-06T18:23:15Z

packages/optimizely-sdk/lib/index.browser.ts


+import { LocalStoragePendingEventsDispatcher } from '@optimizely/js-sdk-event-processor';


why left blank line?

Its probably a line break between module imports and internal imports

msohailhussain · 2020-10-06T18:27:30Z

packages/optimizely-sdk/lib/index.browser.ts

+
+let hasRetriedEvents = false;
+
+interface Config {


document it. we should export as well? not sure.

msohailhussain · 2020-10-06T18:28:30Z

packages/optimizely-sdk/lib/index.browser.ts

@@ -97,7 +104,7 @@ var createInstance = function(config) {
      eventDispatcher = config.eventDispatcher;
    }

-    config = assign(
+    const optimizelyConfig = assign(


why changed to optimizelyConfig. OptimizelyConfig is different than what is expected here.

msohailhussain · 2020-10-06T18:28:48Z

packages/optimizely-sdk/lib/index.browser.ts

@@ -110,29 +117,29 @@ var createInstance = function(config) {
        logger: logger,
        errorHandler: getErrorHandler(),
      }
-    );
+    ) as OptimizelyConfig;


this is not correct.

msohailhussain · 2020-10-06T18:36:25Z

packages/optimizely-sdk/lib/index.browser.ts

 */
-var createInstance = function(config) {
+const createInstance = function (config: Config): Optimizely | null {


function(config: Config) should be without space.

msohailhussain · 2020-10-06T18:38:03Z

packages/optimizely-sdk/lib/index.react_native.ts

+const DEFAULT_EVENT_FLUSH_INTERVAL = 1000; // Unit is ms, default is 1s
+const DEFAULT_EVENT_MAX_QUEUE_SIZE = 10000;
+
+interface Config {


this is being used in all index level files, we should move it to shared_types.

msohailhussain · 2020-10-06T18:43:07Z

packages/optimizely-sdk/lib/shared_types.ts

@@ -77,6 +79,26 @@ export interface OptimizelyVariable {
  value: string;
 }

+export interface OptimizelyConfig {


Isn't it defined here.
Please use the right term. [OptimizelyConfig](https://github.com/optimizely/javascript-sdk/blob/master/packages/optimizely-sdk/lib/core/optimizely_config/index.js) is different.

msohailhussain

please address.

msohailhussain · 2020-10-06T22:09:48Z

packages/optimizely-sdk/lib/index.browser.ts

@@ -149,7 +140,7 @@ var createInstance = function(config) {
  }
 };

-var __internalResetRetryState = function() {
+const __internalResetRetryState = function (): void {


we have default indentation without space please fix at all places.

msohailhussain · 2020-10-06T22:11:05Z

packages/optimizely-sdk/lib/index.react_native.ts

 import * as enums from './utils/enums';
 import { assign } from './utils/fns';
 import Optimizely from './optimizely';
-import configValidator from './utils/config_validator';
+import * as configValidator from './utils/config_validator';


why not modified in other files?

msohailhussain · 2020-10-06T22:11:52Z

packages/optimizely-sdk/lib/shared_types.ts

@@ -77,6 +79,39 @@ export interface OptimizelyVariable {
  value: string;
 }

+export interface Config {


add documentation for both Config and ConfigObj

zashraf1985

Looks good overall. I have some concerns on the naming of certain variables and Interfaces. Secondly, as a general rule of typescript conversion. Please feel free to simplify some code to use latest javascript constructs. A lot of code can be modernized and simplified as now we are using typescript. For example: using spread operators instead of assign, using arrow functions where they make code look simpler etc.

zashraf1985 · 2020-10-07T16:07:40Z

packages/optimizely-sdk/lib/index.browser.ts


+import { LocalStoragePendingEventsDispatcher } from '@optimizely/js-sdk-event-processor';


Its probably a line break between module imports and internal imports

zashraf1985 · 2020-10-07T16:08:56Z

packages/optimizely-sdk/lib/index.browser.ts

@@ -97,7 +88,7 @@ var createInstance = function(config) {
      eventDispatcher = config.eventDispatcher;
    }

-    config = assign(
+    const internalConfig = assign(


Since you are now in TS, you dont need to use assign. you can probably use spread syntax to override values.

zashraf1985 · 2020-10-07T16:11:40Z

packages/optimizely-sdk/lib/index.browser.ts

@@ -97,7 +88,7 @@ var createInstance = function(config) {
      eventDispatcher = config.eventDispatcher;
    }

-    config = assign(
+    const internalConfig = assign(


variable name can be better. modifiedConfig or updatedConfig. You can try thinking of a better name but internalConfig kind of looks misleading.

zashraf1985 · 2020-10-07T16:12:21Z

packages/optimizely-sdk/lib/index.browser.ts

@@ -110,29 +101,29 @@ var createInstance = function(config) {
        logger: logger,
        errorHandler: getErrorHandler(),
      }
-    );
+    ) as ConfigObj;


This conversions as ConfigObj might not be needed. Are you sure its throwing error without this.?

zashraf1985 · 2020-10-07T16:15:07Z

packages/optimizely-sdk/lib/index.node.ts

@@ -83,7 +73,7 @@ var createInstance = function(config) {
      config.isValidInstance = false;
    }

-    config = assign(
+    const internalConfig = assign(


You can use spread operator here too instead of assign. Also rename internalConfig to something better

zashraf1985 · 2020-10-07T16:23:09Z

packages/optimizely-sdk/lib/shared_types.ts

+/**
+ * Config required to create optimizely object
+ */
+export interface ConfigObj {


I see why you need two different configs/options but ConfigObj is not a good name. Two sets of feedback on this one.

One can be inherited from the other because outer options are a subset of internal options.

Names should be relevant. Config makes sense if its the only interface we use. ConfigObj does not make sense at all. And when we have two different objects with many similar properties, both Config and ConfigObj look like inappropriate choices. One choice could be SDKOptions for the outer interface and OptimizelyOptions for the internal . You can give it a thought and try to come up with better and more appropriate names.

zashraf1985 · 2020-10-07T16:25:37Z

packages/optimizely-sdk/lib/index.react_native.ts

- * @param  {string}         config.sdkKey
- * @return {Object}         the Optimizely object
+ * @param  {Config} config
+ * @return {Optimizely} the Optimizely object


this doc comment should be updated to reflect that it can also return null.

yavorona · 2020-10-09T02:52:17Z

packages/optimizely-sdk/lib/index.browser.ts

+      errorHandler: getErrorHandler(),
+    };
+
+    const modifiedConfig = {...additionalEntities, ...config, ...optimizelyLoggers}  as OptimizelyOptions;


I think we can avoid using as type cast here by updating isValidInstance in SDKOptions object to not be optional.

yavorona · 2020-10-09T02:56:40Z

packages/optimizely-sdk/lib/index.browser.ts

        window.addEventListener(
          unloadEvent,
-          function() {
+          function () {


Recommend using arrow function here

msohailhussain · 2020-10-14T18:58:13Z

packages/optimizely-sdk/lib/index.browser.ts

+      errorHandler: getErrorHandler(),
+    };
+
+    const modifiedConfig = {...additionalEntities, ...config, ...optimizelyLoggers};


updatedConfig

the names additionalEntities and optimizelyLoggers do not make much sense. Instead of building interim objects, you can add those directly when building the modifiedConfig or updatedConfig object. something like

const modifiedConfig = { clientEngine: enums.JAVASCRIPT_CLIENT_ENGINE, eventBatchSize: DEFAULT_EVENT_BATCH_SIZE, eventFlushInterval: DEFAULT_EVENT_FLUSH_INTERVAL, eventDispatcher: eventDispatcher, ...config, logger: logger, errorHandler: getErrorHandler(), }

Also you can probably name it optimizelyOptions instead of modifiedConfig

msohailhussain · 2020-10-14T18:58:33Z

packages/optimizely-sdk/lib/index.browser.ts

-        errorHandler: getErrorHandler(),
-      }
-    );
+    const additionalEntities = {


need to put more appropriate name.

zashraf1985

Detailed comments provided in browser entry file also apply to the other two entry points. I have NOT duplicated comments to all three entry points.

zashraf1985 · 2020-10-14T19:06:18Z

packages/optimizely-sdk/lib/index.browser.ts

+      errorHandler: getErrorHandler(),
+    };
+
+    const modifiedConfig = {...additionalEntities, ...config, ...optimizelyLoggers};


the names additionalEntities and optimizelyLoggers do not make much sense. Instead of building interim objects, you can add those directly when building the modifiedConfig or updatedConfig object. something like

const modifiedConfig = { clientEngine: enums.JAVASCRIPT_CLIENT_ENGINE, eventBatchSize: DEFAULT_EVENT_BATCH_SIZE, eventFlushInterval: DEFAULT_EVENT_FLUSH_INTERVAL, eventDispatcher: eventDispatcher, ...config, logger: logger, errorHandler: getErrorHandler(), }

zashraf1985 · 2020-10-14T19:08:10Z

packages/optimizely-sdk/lib/index.browser.ts


    if (!eventProcessorConfigValidator.validateEventBatchSize(config.eventBatchSize)) {
      logger.warn('Invalid eventBatchSize %s, defaulting to %s', config.eventBatchSize, DEFAULT_EVENT_BATCH_SIZE);
-      config.eventBatchSize = DEFAULT_EVENT_BATCH_SIZE;
+      modifiedConfig.eventBatchSize = DEFAULT_EVENT_BATCH_SIZE;


declaring this object above with such detail and then mutating some of the elements makes it confusing. more cleaner code could be to assign eventBatchSize to a local variable and then declare the optimizelyOptions once at the end.

zashraf1985 · 2020-10-14T19:08:27Z

packages/optimizely-sdk/lib/index.browser.ts

    }
    if (!eventProcessorConfigValidator.validateEventFlushInterval(config.eventFlushInterval)) {
      logger.warn(
        'Invalid eventFlushInterval %s, defaulting to %s',
        config.eventFlushInterval,
        DEFAULT_EVENT_FLUSH_INTERVAL
      );
-      config.eventFlushInterval = DEFAULT_EVENT_FLUSH_INTERVAL;
+      modifiedConfig.eventFlushInterval = DEFAULT_EVENT_FLUSH_INTERVAL;


The same comment about eventBatchSize applies here too

zashraf1985 · 2020-10-14T19:24:27Z

packages/optimizely-sdk/lib/index.browser.ts


    try {
      if (typeof window.addEventListener === 'function') {
-        var unloadEvent = 'onpagehide' in window ? 'pagehide' : 'unload';
+        const unloadEvent = 'onpagehide' in window ? 'pagehide' : 'unload';


Nonblocking: Can we move these string values somewhere inside constants

zashraf1985 · 2020-10-14T19:25:22Z

packages/optimizely-sdk/lib/index.browser.ts

+          () => {
            optimizely.close();
          },


Suggested change

() => {

optimizely.close();

},

optimizely.close,

zashraf1985 · 2020-10-14T19:31:22Z

packages/optimizely-sdk/lib/index.browser.ts

+      errorHandler: getErrorHandler(),
+    };
+
+    const modifiedConfig = {...additionalEntities, ...config, ...optimizelyLoggers};


Also you can probably name it optimizelyOptions instead of modifiedConfig

zashraf1985 · 2020-10-14T19:34:48Z

packages/optimizely-sdk/lib/shared_types.ts

+}
+
+/**
+ * Config required to create optimizely object


Probably change it to Options required

zashraf1985 · 2020-10-14T19:39:02Z

packages/optimizely-sdk/lib/shared_types.ts

+export interface SDKOptions {
+  datafile?: string;
+  errorHandler?: ErrorHandler;
+  eventDispatcher?: EventDispatcher;
+  logger?: LogHandler;
+  logLevel?: LogLevel;
+  userProfileService?: UserProfileService;
+  eventBatchSize?: number;
+  eventFlushInterval?: number;
+  sdkKey?: string;
+  isValidInstance: boolean;
+}


I believe we should probably add one line documentation comment to each option here. This might help while autogenerating docs

zashraf1985 · 2020-10-14T19:44:23Z

packages/optimizely-sdk/lib/shared_types.ts

+  clientEngine: string;
+  clientVersion?: string;
+  errorHandler: ErrorHandler;
+  eventDispatcher: EventDispatcher;
+  isValidInstance: boolean;
+  datafile?: string;
+  // TODO[OASIS-6649]: Don't use object type
+  // eslint-disable-next-line  @typescript-eslint/ban-types
+  jsonSchemaValidator?: object;
+  sdkKey?: string;
+  userProfileService?: UserProfileService | null;
+  UNSTABLE_conditionEvaluators?: unknown;
+  eventFlushInterval?: number;
+  eventBatchSize?: number;
+  datafileOptions?: DatafileOptions;
+  eventMaxQueueSize?: number;
+  logger: LogHandler;


NIT: Sort them alphabetically maybe?

zashraf1985 · 2020-10-14T19:44:31Z

packages/optimizely-sdk/lib/shared_types.ts

+  datafile?: string;
+  errorHandler?: ErrorHandler;
+  eventDispatcher?: EventDispatcher;
+  logger?: LogHandler;
+  logLevel?: LogLevel;
+  userProfileService?: UserProfileService;
+  eventBatchSize?: number;
+  eventFlushInterval?: number;
+  sdkKey?: string;
+  isValidInstance: boolean;


NIT: Sort them alphabetically maybe?

mjc1283

Mostly LGTM, two small requests

mjc1283 · 2020-10-16T16:47:33Z

packages/optimizely-sdk/lib/index.browser.ts

-          function() {
-            optimizely.close();
-          },
+          optimizely.close,


Have you tested this change? I think it won't work. close is not bound to optimizely, so when it is invoked it may not have the right this value.

Good catch! I asked uzair to change it this way to make it look simple and clean but it didn't occur to me that it will mess up with this. @ozayr-zaviar ! please change it back to what it was or an arrow function whatever makes sense.

mjc1283 · 2020-10-16T16:52:29Z

packages/optimizely-sdk/lib/shared_types.ts

+export interface OptimizelyOptions {
+  UNSTABLE_conditionEvaluators?: unknown;
+  clientEngine: string;
+  clientVersion?: string;
+  datafile?: string;
+  datafileOptions?: DatafileOptions;
+  errorHandler: ErrorHandler;
+  eventBatchSize?: number;
+  eventDispatcher: EventDispatcher;
+  eventFlushInterval?: number;
+  eventMaxQueueSize?: number;
+  isValidInstance: boolean;
+  // TODO[OASIS-6649]: Don't use object type
+  // eslint-disable-next-line  @typescript-eslint/ban-types
+  jsonSchemaValidator?: object;
+  logger: LogHandler;
+  sdkKey?: string;
+  userProfileService?: UserProfileService | null;
+}


I think OptimizelyOptions should be defined in lib/optimizely/index.ts.

msohailhussain

lgtm

zashraf1985

LGTM! Great Work @ozayr-zaviar

converted entry level modules to ts

f47a697

optimizely-codereview-bot assigned ozayr-zaviar Oct 6, 2020

ozayr-zaviar removed their assignment Oct 6, 2020

msohailhussain suggested changes Oct 6, 2020

View reviewed changes

uzair-folio3 added 5 commits October 7, 2020 00:21

comments addressed: fixed linting and add config in shared_types

a2b25ba

lint fixed in index.browser.js

793b283

removed extra spaces

bb9468c

removed configObject to shared_types and renamed variables

24386fb

ConfigObj naming convention fixed

a38d13d

msohailhussain suggested changes Oct 6, 2020

View reviewed changes

msohailhussain marked this pull request as ready for review October 6, 2020 22:12

msohailhussain requested a review from a team as a code owner October 6, 2020 22:12

msohailhussain requested review from zashraf1985, yavorona and mjc1283 October 6, 2020 22:13

optimizely-codereview-bot added the 🤖 4 hrs label Oct 6, 2020

comments addressed

1a9f54f

optimizely-codereview-bot added 🤖 8 hrs and removed 🤖 4 hrs labels Oct 7, 2020

zashraf1985 suggested changes Oct 7, 2020

View reviewed changes

optimizely-codereview-bot added 🤖 12 hrs and removed 🤖 8 hrs labels Oct 7, 2020

comments addressed

5fe9b80

optimizely-codereview-bot added 🤖 16 hrs 🤖 20 hrs and removed 🤖 12 hrs 🤖 16 hrs labels Oct 8, 2020

yavorona reviewed Oct 9, 2020

View reviewed changes

optimizely-codereview-bot assigned ozayr-zaviar Oct 14, 2020

optimizely-codereview-bot removed the 🤖 24 hrs label Oct 14, 2020

Merge branch 'master' into uzair/entry_level_modules_to_ts

356ae1b

msohailhussain reviewed Oct 14, 2020

View reviewed changes

msohailhussain requested review from zashraf1985 and yavorona October 14, 2020 19:00

msohailhussain unassigned ozayr-zaviar Oct 14, 2020

optimizely-codereview-bot requested a review from msohailhussain October 14, 2020 19:00

zashraf1985 suggested changes Oct 14, 2020

View reviewed changes

optimizely-codereview-bot added the 🤖 4 hrs label Oct 14, 2020

uzair-folio3 added 2 commits October 15, 2020 12:30

exported optimizelyOptions for javascript-testapp

a47a7c0

comments addressed

f6a626e

optimizely-codereview-bot added 🤖 8 hrs 🤖 12 hrs and removed 🤖 4 hrs 🤖 8 hrs labels Oct 15, 2020

msohailhussain requested a review from zashraf1985 October 16, 2020 05:42

mjc1283 suggested changes Oct 16, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 12 hrs label Oct 16, 2020

optimizely-codereview-bot assigned ozayr-zaviar Oct 16, 2020

uzair-folio3 added 2 commits October 19, 2020 19:29

comment addressed

eaeff08

All comments addressed

e85f578

ozayr-zaviar requested a review from mjc1283 October 19, 2020 17:49

msohailhussain unassigned ozayr-zaviar Oct 19, 2020

mjc1283 approved these changes Oct 19, 2020

View reviewed changes

msohailhussain approved these changes Oct 19, 2020

View reviewed changes

zashraf1985 approved these changes Oct 19, 2020

View reviewed changes

mjc1283 merged commit 6c62080 into master Oct 19, 2020

mjc1283 deleted the uzair/entry_level_modules_to_ts branch October 19, 2020 18:51


		import { LocalStoragePendingEventsDispatcher } from '@optimizely/js-sdk-event-processor';

converted entry level modules to ts #590

converted entry level modules to ts #590

Uh oh!

Conversation

ozayr-zaviar commented Oct 6, 2020

Summary

Test plan

Uh oh!

coveralls commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zashraf1985 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yavorona Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zashraf1985 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

coveralls commented Oct 6, 2020 •

edited

Loading

yavorona Oct 9, 2020 •

edited

Loading

msohailhussain Oct 14, 2020 •

edited

Loading