feat: Convert optimizely module to TS #576

yavorona · 2020-09-25T06:06:37Z

Summary

Convert optimizely module from JS to TS.

Test plan

Existing unit tests and local bundle

coveralls · 2020-09-26T08:17:48Z

Coverage decreased (-0.008%) to 96.63% when pulling 01a9e5d on pnguen/optimizely-module-to-ts into 405bdd0 on master.

yavorona · 2020-09-28T20:49:10Z

packages/optimizely-sdk/lib/shared_types.ts

-  // eslint-disable-next-line  @typescript-eslint/no-explicit-any
-  params: any;
+/**
+ * Temprorary placement of LogTierV1EventProcessorConfig


I originally saved LogTierV1EventProcessorConfig interface in event-processor/src/eventProcessor.ts, which resulted in travis lint failure due to event-processor package not being set up https://travis-ci.org/github/optimizely/javascript-sdk/builds/731087456, so I moved it here for now

Hmm, well I expect event-processor to be providing its own type definitions for what it accepts. Not sure why we are re-defining it here. But in any case, event-processor is published and consumed separately from optimizely-sdk, so we can't make changes in it and immediately pick them up here.

yavorona · 2020-09-28T20:55:05Z

packages/optimizely-sdk/lib/core/project_config/index.js

 };

 /**
 * Determine for given experiment if event is running, which determines whether should be dispatched or not
 */
 export var isRunning = function(projectConfig, experimentKey) {
-  return this.getExperimentStatus(projectConfig, experimentKey) === EXPERIMENT_RUNNING_STATUS;
+  return getExperimentStatus(projectConfig, experimentKey) === EXPERIMENT_RUNNING_STATUS;


existed previously this.... resulted in browser test failure. I do not think we needed it here in the first place. LMKWYT @mjc1283

I agree with this change!

mjc1283

I recommended some small refactors to modernize the code
shared_types is getting pretty big, which could be a sign of poor organization. Are there any types in there that should live in the modules that they're used in?
The as type casts on the return value in the feature variable getter methods - there might be a way to make this more elegant using user-defined type guard functions. But no need to spend a lot of time on that rabbit hole right now.

mjc1283 · 2020-09-28T22:03:58Z

packages/optimizely-sdk/lib/core/project_config/index.js

 };

 /**
 * Determine for given experiment if event is running, which determines whether should be dispatched or not
 */
 export var isRunning = function(projectConfig, experimentKey) {
-  return this.getExperimentStatus(projectConfig, experimentKey) === EXPERIMENT_RUNNING_STATUS;
+  return getExperimentStatus(projectConfig, experimentKey) === EXPERIMENT_RUNNING_STATUS;


I agree with this change!

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

mjc1283 · 2020-09-28T22:08:24Z

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

-  // An event to be submitted to Optimizely, enabling tracking the reach and impact of
-  // tests and feature rollouts.
-  export interface Event {
-    // URL to which to send the HTTP request.
-    url: string;
-    // HTTP method with which to send the event.
-    httpVerb: 'POST';
-    // Value to send in the request body, JSON-serialized.
-    // TODO[OASIS-6649]: Don't use any type
-    // eslint-disable-next-line  @typescript-eslint/no-explicit-any
-    params: any;
-  }
-
-  export interface EventDispatcher {
-    /**
-     * @param event
-     *        Event being submitted for eventual dispatch.
-     * @param callback
-     *        After the event has at least been queued for dispatch, call this function to return
-     *        control back to the Client.
-     */
-    dispatchEvent: (event: Event, callback: () => void) => void;
-  }
-


We can't remove these things from our published interface.

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

mjc1283 · 2020-09-28T22:09:06Z

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

-  interface DatafileOptions {
-    autoUpdate?: boolean;
-    updateInterval?: number;
-    urlTemplate?: string;
-    datafileAccessToken?: string;
-  }
-


We can't remove this from our published interface.

mjc1283 · 2020-09-28T23:24:18Z

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

+    const timeoutId = this.__nextReadyTimeoutId;
+    this.__nextReadyTimeoutId++;
+
+    const onReadyTimeout = function(this: Optimizely) {


Recommend arrow function

mjc1283 · 2020-09-28T23:24:36Z

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

+      }.bind(this)
+    );
+
+    return Promise.race([this.__readyPromise, timeoutPromise]) as Promise<{ success: boolean; reason?: string | undefined; }>;


Recommend removing as type cast

mjc1283 · 2020-09-28T23:25:02Z

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

+      }
+
+      // remove null values from eventTags
+      eventTags = this.__filterEmptyValues(eventTags as EventTags);


Recommend removing as type cast

I removed it by updating the signature of the __filterEmptyValues to

__filterEmptyValues(map: EventTags | undefined): EventTags | undefined { ... }

mjc1283 · 2020-09-28T23:45:22Z

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

+   * Helper method to get the non type-casted value for a variable attached to a
+   * feature flag. Returns appropriate variable value depending on whether there
+   * was a matching variation, feature was enabled or not or varible was part of the
+   * available variation or not. Also logs the appropriate message explaining how it
+   * evaluated the value of the variable.
+   *
+   * @param  {string}          featureKey           Key of the feature whose variable's value is
+   *                                                being accessed
+   * @param  {boolean}         featureEnabled       Boolean indicating if feature is enabled or not
+   * @param  {Variation}       variation            variation returned by decision service
+   * @param  {FeatureVariable} variable             varible whose value is being evaluated
+   * @param  {string}          userId               ID for the user
+   * @return {string|null}                          String value of the variable or null if the
+   *                                                config Obj is null


Interesting - looks like at some point, this function was changed, but they didn't update the comment. It is not returning string | null, it is actually doing the type cast (calling projectConfig.getTypeCastValue). Probably the right return type is unknown.

mjc1283 · 2020-09-28T23:50:55Z

packages/optimizely-sdk/lib/core/project_config/index.d.ts

+ * @returns {T}                               Variable value of the appropriate type, or
+ *                                            null if the type cast failed
+ */
+export function getTypeCastValue<T>(variableValue: string, type: string, logger: LogHandler): T;


Looking a little closer at getTypeCastValue, I think this could be improved (not necessary to do in this PR):

The second argumenttype can be defined as an enum.

Not sure the generic T is helpful here. I could see it helping if we wanted to restrict it to the feature variable types like T extends string | number | boolean, but with JSON variables, we're calling JSON.parse, the return type of which is broader. I would go with unknown instead of the generic T.

yavorona · 2020-09-30T18:53:59Z

I recommended some small refactors to modernize the code

shared_types is getting pretty big, which could be a sign of poor organization. Are there any types in there that should live in the modules that they're used in?

The as type casts on the return value in the feature variable getter methods - there might be a way to make this more elegant using user-defined type guard functions. But no need to spend a lot of time on that rabbit hole right now.

The biggest part of the shared_files contains Optimizely Config Entities, which originally lived in our published index.d.ts file. If I keep it there, we would not be able to access it in our optimizely module where is it being used. Same happens if I move Optimizely Config Entities to project_config_manager, where it is also being used. @mjc1283 LMK if you think there is a better place for them to live.
 I haven’t found a good way to avoid using as type casts in our feature variable getters, but I will spend more time to think about it!

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

mjc1283

Looks really good!

mjc1283 · 2020-10-01T20:03:53Z

packages/optimizely-sdk/lib/core/event_builder/index.d.ts

@@ -16,7 +16,7 @@
 import { ProjectConfig } from '../project_config';
 import { LogHandler } from '@optimizely/js-sdk-logging';
 import { EventTags, UserAttributes } from '../../shared_types';
-import { Event as EventLoggingEndpoint } from '../../shared_types';
+import { Event as EventLoggingEndpoint } from '@optimizely/optimizely-sdk';


Event should stay in shared_types I think. We were trying to avoid importing from @optimizely/optimizely-sdk within the lower-level modules.

mjc1283 · 2020-10-01T20:04:41Z

packages/optimizely-sdk/lib/core/notification_center/index.d.ts

-  Experiment,
-  Variation
-} from '../../shared_types';
+import { Event } from '@optimizely/optimizely-sdk';


Event should stay in shared_types I think. We were trying to avoid importing from @optimizely/optimizely-sdk within the lower-level modules.

mjc1283 · 2020-10-01T20:11:51Z

packages/optimizely-sdk/lib/core/project_config/entities.ts

+  id: string;
+  key: string;
+  status: string;
+  layerId: string;
+  variations: Variation[];
+  trafficAllocation: Array<{
+    entityId: string;
+    endOfRange: number;
+  }>;
+  audienceIds: string[];
+  // TODO[OASIS-6649]: Don't use object type
+  // eslint-disable-next-line  @typescript-eslint/ban-types
+  forcedVariations: object;
+  variationKeyMap: {[key: string]: Variation}


I would remove all these properties except id, key, and variationKeyMap. We could add them later when we need them, but it appears only these 3 are needed at the moment (used by the code in the optimizely module).

mjc1283 · 2020-10-01T20:15:24Z

packages/optimizely-sdk/lib/core/project_config/project_config_manager.d.ts

+interface ProjectConfigManagerConfig {
+  // TODO[OASIS-6649]: Don't use object type
+  // eslint-disable-next-line  @typescript-eslint/ban-types
+  datafile: object | string,


I believe this is an optional string (the doc comment is wrong):

Suggested change

datafile: object | string,

datafile?: string,

mjc1283 · 2020-10-01T20:17:07Z

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

@@ -1,5 +1,5 @@
 /**
- * Copyright 2018-2020, Optimizely
+ * Copyright 2020, Optimizely


This has to stay 2018-2020. 2020 alone should be used for files created during 2020.

mjc1283 · 2020-10-01T20:37:18Z

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

+    let resolveTimeoutPromise: (value?: { success: boolean; reason?: string | undefined; }) => void;
+    const timeoutPromise = new Promise(
+      function(resolve: (value?: { success: boolean; reason?: string | undefined; }) => void) {


Idea for making this less verbose: the eventual value of Promises returned by onReady can be defined as an interface in shared_types:

interface OnReadyResult { success: boolean; reason?: string; }

This can be used here, and also in lib/index.d.ts

Suggested change

let resolveTimeoutPromise: (value?: { success: boolean; reason?: string | undefined; }) => void;

const timeoutPromise = new Promise(

function(resolve: (value?: { success: boolean; reason?: string | undefined; }) => void) {

let resolveTimeoutPromise: (value: OnReadyResult) => void;

const timeoutPromise = new Promise<OnReadyResult>(

(resolve) => {

mjc1283 · 2020-10-01T20:38:12Z

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

      resolveTimeoutPromise({
        success: false,
        reason: sprintf('onReady timeout expired after %s ms', timeoutValue),
      });
-    }.bind(this);
+    }).bind(this);


Calling .bind is not necessary here.

mjc1283 · 2020-10-01T21:00:19Z

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

+import { sprintf, objectValues } from '@optimizely/js-sdk-utils';
+import { LogHandler, ErrorHandler } from '@optimizely/js-sdk-logging';
+import { FeatureFlag, FeatureVariable } from '../core/project_config/entities';
+import { EventDispatcher } from '@optimizely/js-sdk-event-processor';


EventDispatcher is already defined in lib/index.d.ts, let's move it to shared_types and import it from there.

Found incompatibility in our EventDispatcher interface with the one defined in event processor.
The callback in dispatchEvent method is updated from callback: () => void to callback: (response: { statusCode: number; }) => void. I noted this change in the log.

mjc1283 · 2020-10-01T22:45:16Z

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

+   *
+   */
+  private validateInputs(
+    stringInputs?: unknown,


Looking at the callers, it appears we always pass an object. This will simplify the implementation:

Suggested change

stringInputs?: unknown,

// Later we could make these camelCase, snake_case is weird

stringInputs: Record<'feature_key' | 'user_id' | 'variable_key' | 'experiment_key', unknown>

Made some modification to avoid missing properties compiler error and added event_key:
Partial<Record<'feature_key' | 'user_id' | 'variable_key' | 'experiment_key' | 'event_key', unknown>>

mjc1283

Great work!

mjc1283 · 2020-10-05T17:57:46Z

packages/optimizely-sdk/lib/core/notification_center/index.d.ts

+export enum NOTIFICATION_TYPES {
+  ACTIVATE = 'ACTIVATE:experiment, user_id,attributes, variation, event',
+  DECISION = 'DECISION:type, userId, attributes, decisionInfo',
+  LOG_EVENT = 'LOG_EVENT:logEvent',
+  OPTIMIZELY_CONFIG_UPDATE = 'OPTIMIZELY_CONFIG_UPDATE',
+  TRACK = 'TRACK:event_key, user_id, attributes, event_tags, event',
+}


These are defined in the utils package - does this work?

Suggested change

export enum NOTIFICATION_TYPES {

ACTIVATE = 'ACTIVATE:experiment, user_id,attributes, variation, event',

DECISION = 'DECISION:type, userId, attributes, decisionInfo',

LOG_EVENT = 'LOG_EVENT:logEvent',

OPTIMIZELY_CONFIG_UPDATE = 'OPTIMIZELY_CONFIG_UPDATE',

TRACK = 'TRACK:event_key, user_id, attributes, event_tags, event',

}

export type NOTIFICATION_TYPES = import('@optimizely/js-sdk-utils').NOTIFICATION_TYPES;

Works great!

mjc1283 · 2020-10-05T19:11:09Z

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

+  ): boolean {
+    try {
+      // Null, undefined or non-string user Id is invalid.
+      if (typeof stringInputs === 'object' && stringInputs !== null) {


I think this check is not needed anymore - stringInputs has to be a non-null object.

mjc1283 · 2020-10-05T19:16:02Z

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

+    let resolveTimeoutPromise: (value: OnReadyResult) => void;
+    const timeoutPromise = new Promise<OnReadyResult>(
+      (resolve) => {
+      resolveTimeoutPromise = resolve;


Missing indentation

…elper.d.ts

Fix comments Update formatting and add optional attribute sign Clean up Address Matt's comments part 1 Address comment part2 Incorporating commenta part 3 Incorporate comments part 4 Updating names of private methods Update private methods Create configObj interface Clean up Add back DatafileOptions Create ProjectConfigManagerConfig interface Fix LogTierV1EventProcessor compiler error without re-defining LogTierV1EventProcessorConfig Fix EventTags to not except boolean incporporating comments part 1 Incoprorate comments part 2 Returned UserAttributes type back go index.d.ts Return all types back to index.d.ts

yavorona added the WIP label Sep 25, 2020

yavorona requested a review from a team as a code owner September 25, 2020 06:06

yavorona self-assigned this Sep 25, 2020

yavorona removed the WIP label Sep 26, 2020

This was referenced Sep 28, 2020

feat: Converted index.node.js to TS #579

Closed

feat: Converted index.browser.js to TS #580

Closed

feat: Converted index.react_native.js to TS #581

Closed

yavorona force-pushed the pnguen/optimizely-module-to-ts branch 2 times, most recently from f3c51c4 to 687d620 Compare September 28, 2020 19:46

yavorona removed their assignment Sep 28, 2020

yavorona force-pushed the pnguen/optimizely-module-to-ts branch from d1ecbb0 to a2cefd7 Compare September 28, 2020 20:39

yavorona commented Sep 28, 2020

View reviewed changes

mjc1283 suggested changes Sep 28, 2020

View reviewed changes

optimizely-codereview-bot assigned yavorona Sep 28, 2020

yavorona removed their assignment Sep 30, 2020

optimizely-codereview-bot requested a review from mjc1283 September 30, 2020 18:54

optimizely-codereview-bot added the 🤖 4 hrs label Sep 30, 2020

yavorona commented Oct 1, 2020

View reviewed changes

packages/optimizely-sdk/lib/index.d.ts Outdated Show resolved Hide resolved

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

mjc1283 suggested changes Oct 1, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 8 hrs label Oct 1, 2020

optimizely-codereview-bot assigned yavorona Oct 1, 2020

This was referenced Oct 2, 2020

feat: Converted index.react_native.js to TS #583

Closed

feat: Converted index.browser.js to TS #585

Closed

feat: Converted index.node.js to TS #584

Closed

yavorona force-pushed the pnguen/optimizely-module-to-ts branch from 3ccb52f to e1ac24a Compare October 2, 2020 19:52

yavorona force-pushed the pnguen/optimizely-module-to-ts branch from e1ac24a to 7de389e Compare October 2, 2020 20:05

yavorona removed their assignment Oct 2, 2020

optimizely-codereview-bot requested a review from mjc1283 October 2, 2020 20:14

ozayr-zaviar mentioned this pull request Oct 5, 2020

feat: Converted entry level modules to TS #586

Closed

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

mjc1283 approved these changes Oct 5, 2020

View reviewed changes

uzair-folio3 and others added 6 commits October 5, 2020 12:23

added declaration file for notification center

591bf0d

comment addressed: seperate declaration file for event_helper event_h…

7c9af5c

…elper.d.ts

event_helpers module name and declaration file name corrected

d52bdd2

EOL in event_helpers.d.ts

24baa9e

Optimize imports and rebase

f09a046

yavorona force-pushed the pnguen/optimizely-module-to-ts branch from 8da7267 to f09a046 Compare October 5, 2020 19:35

yavorona added 3 commits October 5, 2020 13:24

Optimize exports

a1cb29e

Optimize NOTIFICATION_TYPES export

ef3d200

Fix indentation and remove unnecessary if statement

01a9e5d

yavorona merged commit c79e772 into master Oct 5, 2020

yavorona deleted the pnguen/optimizely-module-to-ts branch October 5, 2020 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Convert optimizely module to TS #576

feat: Convert optimizely module to TS #576

yavorona commented Sep 25, 2020

coveralls commented Sep 26, 2020 •

edited

Loading

yavorona Sep 28, 2020 •

edited

Loading

mjc1283 Sep 28, 2020

yavorona Sep 28, 2020 •

edited

Loading

mjc1283 Sep 28, 2020

mjc1283 left a comment

mjc1283 Sep 28, 2020

mjc1283 Sep 28, 2020

mjc1283 Sep 28, 2020

mjc1283 Sep 28, 2020

mjc1283 Sep 28, 2020

mjc1283 Sep 28, 2020

yavorona Sep 29, 2020

mjc1283 Sep 28, 2020

mjc1283 Sep 28, 2020

yavorona commented Sep 30, 2020 •

edited

Loading

mjc1283 left a comment

mjc1283 Oct 1, 2020

mjc1283 Oct 1, 2020

mjc1283 Oct 1, 2020

mjc1283 Oct 1, 2020

mjc1283 Oct 1, 2020

mjc1283 Oct 1, 2020

mjc1283 Oct 1, 2020

mjc1283 Oct 1, 2020

yavorona Oct 2, 2020

mjc1283 Oct 1, 2020

yavorona Oct 2, 2020

mjc1283 left a comment

mjc1283 Oct 5, 2020

yavorona Oct 5, 2020

mjc1283 Oct 5, 2020

mjc1283 Oct 5, 2020

	stringInputs?: unknown,
	// Later we could make these camelCase, snake_case is weird
	stringInputs: Record<'feature_key' \| 'user_id' \| 'variable_key' \| 'experiment_key', unknown>

feat: Convert optimizely module to TS #576

feat: Convert optimizely module to TS #576

Conversation

yavorona commented Sep 25, 2020

Summary

Test plan

coveralls commented Sep 26, 2020 • edited Loading

yavorona Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yavorona Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjc1283 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

yavorona commented Sep 30, 2020 • edited Loading

mjc1283 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

Choose a reason for hiding this comment

mjc1283 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

coveralls commented Sep 26, 2020 •

edited

Loading

yavorona Sep 28, 2020 •

edited

Loading

yavorona Sep 28, 2020 •

edited

Loading

yavorona commented Sep 30, 2020 •

edited

Loading