refact: Integrate code from `utils` package into `optimizely-sdk` by ozayr-zaviar · Pull Request #749 · optimizely/javascript-sdk

ozayr-zaviar · 2022-03-28T05:50:56Z

Summary

Integrated code from @optimizely/js-sdk-utils into @optimizely/optimizely-sdk package to reduce unnecessary external dependency.

Test plan

All existing unit and E2E tests pass.

coveralls · 2022-03-28T09:10:11Z

Coverage decreased (-0.2%) to 97.021% when pulling a1a6033 on uzair/consolidate-packages into 4034c7d on master.

zashraf1985

Looks great Overall! I have a couple of requests other than the explicit comments.

Can you update the copyright header wherever applicable.
Please add description to the PR.

zashraf1985 · 2022-03-28T17:04:22Z

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

+   sendNotifications(notificationType: NOTIFICATION_TYPES, notificationData?: any): void
+ }
+
 export default {


Can you add the new additions to default exports as well.

zashraf1985 · 2022-03-28T17:21:55Z

packages/optimizely-sdk/rollup.config.js

    typescript(typescriptPluginOptions),
  ],
-  external: ['json-schema', '@optimizely/js-sdk-utils'],
+  external: ['json-schema', './lib/utils/fns', 'uuid'],


./libs/utils/fns should not be needed as an external bundle here

zashraf1985

This is getting really close. A couple of more changes to go.

Move NotificationCenter interface and types from utils to NotificationCenter module.
Copyright should be updated to reflect only the years in which the file was changed. For example a file containing 2016, 2018 will be changed to 2016, 2018, 2022. A file containing 2016, 2018-2021 will be changed to 2016, 2018-2022. Use commas to separate the years where there is a break in between. Use dashes where there is continuity.

zashraf1985 · 2022-04-12T23:18:23Z

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

+*    - logEvent {Object}
+*
+*/
+export enum NOTIFICATION_TYPES {


NOTIFICATION_TYPES and NotificationCenter interface can be moved to NotificationCenter module?

zashraf1985 · 2022-04-13T19:39:11Z

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


 const MODULE_NAME = 'NOTIFICATION_CENTER';

+export enum NOTIFICATION_TYPES {


move this to utils/enums.ts

zashraf1985 · 2022-04-13T19:39:30Z

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

  return new NotificationCenter(options);
 }
+
+export interface NotificationCenter {


remove this as this interface is already available in shared_types

NotificationCenter in shared type does not have sendNotifications function which is required in Optimizely class. This Notification Center contains sendNotifications

There has to be a name conflict because this file now exports a class and an interface both with the same name. can you fix this

zashraf1985 · 2022-04-28T17:15:55Z

@ozayr-zaviar Can you update this branch with master. Looks like there are some conflicts after you merged your GIT actions PR

opti-jnguyen · 2022-05-04T23:42:10Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.tests.js

 import { assert } from 'chai';
 import { getLogger } from '@optimizely/js-sdk-logging';
-import { sprintf } from '@optimizely/js-sdk-utils';
+import { sprintf } from '../../utils/fns';


Any thoughts on using path aliases in the optimizely-sdk/tsconfig.json file to make these imports a bit easier to read?

Example tsconfig.js:

"paths": { "*": [ "./typings/*" ], "@utils": [ "./lib/utils/*" ], },

Usage:

import { sprint } from '@utils/fns'

opti-jnguyen · 2022-05-04T23:46:24Z

packages/optimizely-sdk/lib/plugins/event_processor/forwarding_event_processor.ts

  ProcessableEvent,
 } from '@optimizely/js-sdk-event-processor';
-import { NotificationCenter } from '@optimizely/js-sdk-utils';
+import { NotificationSender } from '../../core/notification_center';


Same comment as above, we should consider configuring tsconfig.json to add a path alias for the utils directory.

opti-jnguyen · 2022-05-04T23:47:07Z

packages/optimizely-sdk/lib/plugins/event_processor/forwarding_event_processor.ts

+  private NotificationSender?: NotificationSender;

-  constructor(dispatcher: EventDispatcher, notificationCenter?: NotificationCenter) {
+  constructor(dispatcher: EventDispatcher, notificationCenter?: NotificationSender) {


Should we refactor notificationCenter to notificationSender?

opti-jnguyen · 2022-05-04T23:48:09Z

packages/optimizely-sdk/lib/plugins/event_processor/forwarding_event_processor.ts

 }

-export function createForwardingEventProcessor(dispatcher: EventDispatcher, notificationCenter?: NotificationCenter): EventProcessor {
+export function createForwardingEventProcessor(dispatcher: EventDispatcher, notificationCenter?: NotificationSender): EventProcessor {


Consider notificationCenter => notificationSender

opti-jnguyen · 2022-05-04T23:51:15Z

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

+* @param {*} value
+* @returns {boolean}
+*/
+export function isValidEnum(enumToCheck: { [key: string]: any }, value: number | string): boolean {


Is the lint warning here being ignored purposefully?

For now, the warning has been suppressed but the any type will be removed in another refactor activity described in theTODO.

opti-jnguyen

Items to be reviewed:

Path alias implementation for utils (import "@utils").
Should notificationCenter be refactored to notificationSender?
utils/fns/index.ts > Is line 82's linting warning being ignored on purpose?

Items for later:

Consider import order linting.

zashraf1985

@ozayr-zaviar can you update the @utils path alias every where.. i see many files are still using ../../utls

zashraf1985 · 2022-05-16T04:32:46Z

packages/optimizely-sdk/jest.config.js

@@ -0,0 +1,17 @@
+module.exports = {
+  // "roots": [


Remove commented code

zashraf1985 · 2022-05-16T04:37:03Z

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

+    }
+  })
+}
+/*


Move these comments to notificationcenter where you moved the code that was below these comments originally.

…vascript-sdk into uzair/consolidate-packages

zashraf1985

LGTM! Great work @ozayr-zaviar

opti-jnguyen

LGTM 🎉

opti-jnguyen · 2022-05-23T13:18:02Z

Saw latest two commits - LGTM!

ozayr-zaviar added 6 commits March 2, 2022 13:05

utils integrated

92e3ab7

logging integrated

37cab7b

testcases fix

1d5368c

revert logging

82b0fa1

utils merged in optimizely sdk

2843dcc

small changes removed

8574c7e

optimizely-codereview-bot assigned ozayr-zaviar Mar 28, 2022

ozayr-zaviar and others added 9 commits March 28, 2022 10:53

Merge branch 'master' into uzair/consolidate-packages

f725a21

ts config added because jest and mocha use similar declarations

28905d0

type warning fixed

11518e1

more types fixed

d3371ce

fix

da73e11

fix

069e501

fix

bc6d5fb

type defined

3b758bd

type fix

73e1db8

zashraf1985 suggested changes Mar 28, 2022

View reviewed changes

ozayr-zaviar added 5 commits April 12, 2022 20:23

Merge branch 'master' into uzair/consolidate-packages

d41146d

headers updated

550c3ba

fix

39b5d69

correct notification type imported

d00a828

comment addressed

c958cda

zashraf1985 suggested changes Apr 12, 2022

View reviewed changes

headers updated and notification center moved

c36dee9

ozayr-zaviar closed this Apr 13, 2022

ozayr-zaviar reopened this Apr 13, 2022

zashraf1985 closed this Apr 13, 2022

zashraf1985 reopened this Apr 13, 2022

zashraf1985 suggested changes Apr 13, 2022

View reviewed changes

optimizely-codereview-bot removed the 🤖 12 hrs label Apr 26, 2022

optimizely-codereview-bot assigned ozayr-zaviar Apr 26, 2022

comment addressed

a8e7858

Merge branch 'master' into uzair/consolidate-packages

0bb709c

opti-jnguyen reviewed May 4, 2022

View reviewed changes

opti-jnguyen suggested changes May 4, 2022

View reviewed changes

comments addressed

f36baf4

zashraf1985 closed this May 9, 2022

zashraf1985 reopened this May 9, 2022

zashraf1985 closed this May 11, 2022

zashraf1985 reopened this May 11, 2022

Merge branch 'master' into uzair/consolidate-packages

f7e9855

zashraf1985 suggested changes May 13, 2022

View reviewed changes

zashraf1985 suggested changes May 16, 2022

View reviewed changes

ozayr-zaviar and others added 2 commits May 16, 2022 19:29

comments addressed

055860f

Merge branch 'uzair/consolidate-packages' of github.com:optimizely/ja…

5796dbe

…vascript-sdk into uzair/consolidate-packages

zashraf1985 approved these changes May 16, 2022

View reviewed changes

opti-jnguyen approved these changes May 16, 2022

View reviewed changes

ozayr-zaviar and others added 2 commits May 18, 2022 16:25

alias @utils removed

fe25893

renamed a variable

86cbb08

removed utils from dependency and added changelog entry

a1a6033

zashraf1985 changed the title ~~refact: Integrate utils package~~ refact: Integrate code from utils package into optimizely-sdk May 23, 2022

zashraf1985 merged commit 843c9c4 into master May 23, 2022

zashraf1985 deleted the uzair/consolidate-packages branch May 23, 2022 22:56


		const MODULE_NAME = 'NOTIFICATION_CENTER';

		export enum NOTIFICATION_TYPES {

Conversation

ozayr-zaviar commented Mar 28, 2022 • edited by zashraf1985 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coveralls commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

zashraf1985 commented Apr 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opti-jnguyen May 4, 2022 • 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!

opti-jnguyen left a comment

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!

zashraf1985 left a comment

Choose a reason for hiding this comment

Uh oh!

opti-jnguyen left a comment • 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!

opti-jnguyen commented May 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ozayr-zaviar commented Mar 28, 2022 •

edited by zashraf1985

Loading

coveralls commented Mar 28, 2022 •

edited

Loading

opti-jnguyen May 4, 2022 •

edited

Loading

opti-jnguyen left a comment •

edited

Loading