feat: Created declaration file for project config index.d.ts module #567

ozayr-zaviar · 2020-09-15T15:38:02Z

Summary

index.d.ts file added in lib/core/project_config

Testplan

All unit tests must work fine. For now, we don't have any unit tests for declaration file.

coveralls · 2020-09-15T15:41:37Z

Coverage remained the same at 96.638% when pulling 941c5f5 on uzair/project_config_interface into 25c8bd0 on master.

msohailhussain · 2020-09-15T16:20:47Z

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

+ * limitations under the License.
+ */
+
+declare module '@optimizely/optimizely-sdk/lib/core/event_builder' {


Why it's event_builder?

mikeproeng37 · 2020-09-15T16:27:02Z

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

+
+declare module '@optimizely/optimizely-sdk/lib/core/project_config' {
+  import { LogHandler } from '@optimizely/js-sdk-logging';
+  import { Experiment, Variation } from '@optimizely/optimizely-sdk';


I think that importing a root-level module from a module downstream is an anti-pattern. I think it makes more sense to export Experiment and Variation interfaces from ProjectConfig and then import them here from ProjectConfig. Wdyt @mjc1283

@mikeng13 Agreed 👍 !

There might be issues with importing/exporting in declaration files (not TS source files), and sharing them into the root level index.d.ts. I believe @yavorona ran into this with user attributes, and we ended up creating a shared_types.ts file. The goal was to keep definitions identical in shared_types.ts and root index.d.ts.

However, if we can actually keep the definitions where they belong in lower modules like this, and still export them appropriately, I'm all for it.

msohailhussain · 2020-09-15T16:43:31Z

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

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


do we need any?

msohailhussain · 2020-09-15T16:46:10Z

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

+    id: string
+    policy: string
+    trafficAllocation: Array<{
+      entityId: string;


This can be a separate type

interface TrafficAllocationRange { entityId: string; endOfRange: number; } // ... interface Group { trafficAllocation: TrafficAllocationRange[]; }

msohailhussain · 2020-09-15T16:48:55Z

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

+    key: string;
+  }
+
+  interface Variable {


FeatureVariable

msohailhussain · 2020-09-15T16:49:44Z

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

+    defaultValue: string;
+    id: string;
+    key: string;
+    type: string;


we also have subtype as well.

msohailhussain · 2020-09-15T17:08:11Z

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

+    type: string;
+  }
+
+  interface RollOut {


msohailhussain · 2020-09-15T17:09:12Z

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

+
+  interface RollOut {
+    experiments: Experiment[];
+    id: string;


key is missing

msohailhussain · 2020-09-15T17:10:53Z

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

+    revision: string;
+  }
+
+  export interface EventBuilder {


This shouldn't be here, please remove it. It should be in a separate file.

msohailhussain · 2020-09-15T17:20:14Z

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

+  export interface EventBuilder {
+    // TODO[OASIS-6649]: Don't use object type
+    // eslint-disable-next-line  @typescript-eslint/ban-types
+    createMutationSafeDatafileCopy(datafile: object): object;


have some doubt, this should be a part of projectconfig.

Definitely not, this is a function within the project config module, not a method of a ProjectConfig.

This ProjectConfig seems to be confusing ProjectConfig data object with project_config module.

msohailhussain · 2020-09-15T17:20:20Z

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

+    createMutationSafeDatafileCopy(datafile: object): object;
+    // TODO[OASIS-6649]: Don't use object type
+    // eslint-disable-next-line  @typescript-eslint/ban-types
+    createProjectConfig(datafile: object, datafileString: string | null): ProjectConfig


have some doubt, this should be a part of projectconfig.

Agree, should not.

remove this as well.

msohailhussain · 2020-09-15T17:21:16Z

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

+    getVariableValueForVariation(projectConfig: ProjectConfig, variable: Variable, variation: Variation, logger: LogHandler) : string | null
+    // TODO[OASIS-6649]: Don't use any type
+    // eslint-disable-next-line  @typescript-eslint/no-explicit-any
+    getTypeCastValue(variableValue: string, variableType: string, logger: LogHandler): any


have some doubt, this should be a part of projectconfig.

msohailhussain · 2020-09-15T17:21:26Z

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

+    // eslint-disable-next-line  @typescript-eslint/no-explicit-any
+    getTypeCastValue(variableValue: string, variableType: string, logger: LogHandler): any
+    getAudiencesById(projectConfig: ProjectConfig): {[id: string]: Audience}
+    eventWithKeyExists(projectConfig: ProjectConfig, eventKey: string): boolean


have some doubt, this should be a part of projectconfig.

msohailhussain · 2020-09-15T17:21:31Z

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

+    getAudiencesById(projectConfig: ProjectConfig): {[id: string]: Audience}
+    eventWithKeyExists(projectConfig: ProjectConfig, eventKey: string): boolean
+    isFeatureExperiment(projectConfig: ProjectConfig, experimentId: string): boolean
+    toDatafile(projectConfig: ProjectConfig): string


have some doubt, this should be a part of projectconfig.

msohailhussain · 2020-09-15T17:21:36Z

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

+    eventWithKeyExists(projectConfig: ProjectConfig, eventKey: string): boolean
+    isFeatureExperiment(projectConfig: ProjectConfig, experimentId: string): boolean
+    toDatafile(projectConfig: ProjectConfig): string
+    tryCreatingProjectConfig(config: Config): { config: ProjectConfig, error: string | null}


have some doubt, this should be a part of projectconfig.

msohailhussain · 2020-09-21T18:06:30Z

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

+ */
+
+declare module '@optimizely/optimizely-sdk/lib/core/project_config' {
+  // TODO[OASIS-6649]: Don't use any type


there is no any type, please remove it.

mjc1283 · 2020-09-21T18:36:43Z

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

+export type UserAttributes = {
+  // TODO[OASIS-6649]: Don't use any type
+  // eslint-disable-next-line  @typescript-eslint/no-explicit-any
+  [name: string]: any;
+};
+
+export type EventTags = {
+  [key: string]: string | number | boolean;
+};
+
+export type ConditionTree<Leaf> = Leaf | unknown[];


UserAttributes, EventTags, and ConditionTree are not related to project_config and should not be defined here.

UserAttributes is already defined in shared_types.ts.

ConditionTree is already defined in condition_tree.ts.

mjc1283 · 2020-09-21T19:55:41Z

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

-  export type UserAttributes = {
-    // TODO[OASIS-6649]: Don't use any type
-    // eslint-disable-next-line  @typescript-eslint/no-explicit-any
-    [name: string]: any;
-  };
-
-  export type EventTags = {
-    [key: string]: string | number | boolean;
-  };
-


You can't remove these from here, these are part of the public interface used by SDK users.

mjc1283 · 2020-09-21T19:56:06Z

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

-  interface Experiment {
-    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;
-  }
-
-  interface Variation {
-    id: string;
-    key: string;
-  }
-


You can't remove this, this is part of the public interface used by SDK users.

mjc1283 · 2020-09-21T20:00:34Z

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

+export type Condition = {
+  name: string;
+  type: string;
+  match?: string;
+  value: string | number | boolean | null;
+};


This duplicates the definition already existing in custom_attribute_condition_evaluator.ts. It is OK to relocate that definition to this file.

mjc1283 · 2020-09-21T20:07:55Z

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

+declare module '@optimizely/optimizely-sdk/lib/core/project_config' {
+  export interface ProjectConfig {}
+}


Let's add function definitions which will be called or used from lib/optimizely/index.js

msohailhussain · 2020-09-22T16:55:46Z

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

 * limitations under the License.                                           *
 ***************************************************************************/

+import { ConditionTree } from '../project_config/entities';


don't import.

mjc1283 · 2020-09-23T20:16:47Z

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

+   * @return {string} Experiment ID corresponding to the provided experiment key
+   * @throws If experiment key is not in datafile
+   */
+  export function getExperimentId(configObj: ProjectConfig, experimentKey: string): string | never;


Is | never required?

uzair-folio3 added 3 commits September 15, 2020 19:59

index.d.ts added for project_config

cd3a438

Update index.d.ts

9cf0aad

Update index.d.ts

4e0a752

optimizely-codereview-bot assigned ozayr-zaviar Sep 15, 2020

msohailhussain reviewed Sep 15, 2020

View reviewed changes

Update index.d.ts

215c0fe

mikeproeng37 reviewed Sep 15, 2020

View reviewed changes

msohailhussain reviewed Sep 15, 2020

View reviewed changes

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

key: string;

}

interface Variable {

Copy link

Contributor

msohailhussain Sep 15, 2020

Choose a reason for hiding this comment

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

FeatureVariable

msohailhussain reviewed Sep 15, 2020

View reviewed changes

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

type: string;

}

interface RollOut {

Copy link

Contributor

msohailhussain Sep 15, 2020

Choose a reason for hiding this comment

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

Rollout

msohailhussain reviewed Sep 15, 2020

View reviewed changes

Update index.d.ts

ae6186c

msohailhussain marked this pull request as ready for review September 15, 2020 19:05

msohailhussain requested a review from a team as a code owner September 15, 2020 19:05

msohailhussain unassigned ozayr-zaviar Sep 15, 2020

optimizely-codereview-bot requested review from mikeproeng37 and msohailhussain September 15, 2020 19:06

msohailhussain requested review from mjc1283 and removed request for mikeproeng37 and msohailhussain September 15, 2020 19:06

msohailhussain reviewed Sep 21, 2020

View reviewed changes

msohailhussain requested a review from mjc1283 September 21, 2020 18:07

msohailhussain unassigned ozayr-zaviar Sep 21, 2020

optimizely-codereview-bot requested review from mikeproeng37, msohailhussain and yavorona September 21, 2020 18:07

Update index.d.ts

e54a9fc

mjc1283 suggested changes Sep 21, 2020

View reviewed changes

optimizely-codereview-bot assigned ozayr-zaviar Sep 21, 2020

yavorona mentioned this pull request Sep 22, 2020

feat: Added project_config_manager interface #572

Merged

uzair-folio3 added 3 commits September 22, 2020 13:17

restructure

0e1b56c

Update index.d.ts

36c9e62

Update index.d.ts

6add538

msohailhussain reviewed Sep 22, 2020

View reviewed changes

uzair-folio3 added 4 commits September 22, 2020 22:09

revise

fb07216

Update index.ts

0694c7c

Update index.d.ts

5f4d9d4

Merge branch 'master' into uzair/project_config_interface

941c5f5

ozayr-zaviar changed the title ~~index.d.ts file for Project Config added~~ feat: Created declaration file for project config module Sep 22, 2020

msohailhussain changed the title ~~feat: Created declaration file for project config module~~ feat: Created declaration file for project config index.d.ts module Sep 22, 2020

msohailhussain unassigned ozayr-zaviar Sep 22, 2020

optimizely-codereview-bot requested review from mjc1283 and msohailhussain September 22, 2020 18:28

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs and removed 🤖 4 hrs labels Sep 22, 2020

mjc1283 approved these changes Sep 23, 2020

View reviewed changes

mjc1283 merged commit eaf61d0 into master Sep 23, 2020

mjc1283 deleted the uzair/project_config_interface branch September 23, 2020 20:24

feat: Created declaration file for project config index.d.ts module #567

feat: Created declaration file for project config index.d.ts module #567

Uh oh!

Conversation

ozayr-zaviar commented Sep 15, 2020 • edited by msohailhussain Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testplan

Uh oh!

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

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ozayr-zaviar commented Sep 15, 2020 •

edited by msohailhussain

Loading

coveralls commented Sep 15, 2020 •

edited

Loading