refactor: Removed dependency on lodash.forOwn #399

zashraf1985 · 2020-02-18T06:23:56Z

Summary

Removed lodash.forOwn function and replaced all the usaes with Object.keys. This is part of an effort to remove lodash dependency to reduce bundle size.

Test Plan

All Unit tests and Full Stack SDK compatibility passed

coveralls · 2020-02-18T06:37:20Z

Coverage decreased (-0.008%) to 97.331% when pulling 721aef6 on zeeshan/replace-lodash-forown into 59cdad4 on master.

mjc1283 · 2020-02-18T17:03:52Z

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

+  forOwn: function(obj, callback) {
+    for(var key in obj) {
+      if(obj.hasOwnProperty(key)){
+        var val = obj[key];
+        callback(val, key, obj);
+      }
+    }
+  },


This looks good 👍 but could you double check all the places we're using this?

lodash forOwn is more complex, for example:

If you pass null or undefined as the first argument it won't crash

If the callback returns false it breaks out of the loop early

There could be other differences as well. Let's just check all the places we're using it and make sure we are not relying on any of this kind of special functionality.

early return feature is used in some places in the code. I added that functionality and added unit tests.

2. Added unit tests

mjc1283

LGTM, but discussed offline with @zashraf1985. Suggestion is to explore whether we can replace usages with other util FN calls and remove forOwn.

…th Object.keys

mjc1283 · 2020-02-21T16:52:45Z

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

@@ -37,7 +37,9 @@ function NotificationCenter(options) {
  this.logger = options.logger;
  this.errorHandler = options.errorHandler;
  this.__notificationListeners = {};
-  fns.forOwn(enums.NOTIFICATION_TYPES, function(notificationTypeEnum) {
+
+  Object.keys(enums.NOTIFICATION_TYPES).forEach(function(key) {


Suggested change

Object.keys(enums.NOTIFICATION_TYPES).forEach(function(key) {

jsSdkUtils.objectValues(enums.NOTIFICATION_TYPES).forEach(function(notificationTypeEnum) {

mjc1283 · 2020-02-21T17:40:53Z

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

-      if (indexToRemove !== undefined && typeToRemove !== undefined) {
-        return false;
-      }
-    });
-


Using Array.prototype.forEach, we no longer exit early from the loop when false is returned. I suggest Array.prototype.some to maintain the existing behavior.

mjc1283 · 2020-02-21T17:42:54Z

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

@@ -130,7 +131,8 @@ NotificationCenter.prototype.removeNotificationListener = function (listenerId)
 */
 NotificationCenter.prototype.clearAllNotificationListeners = function () {
  try{
-    fns.forOwn(enums.NOTIFICATION_TYPES, function (notificationTypeEnum) {
+    Object.keys(enums.NOTIFICATION_TYPES).forEach(function (key) {


Suggested change

Object.keys(enums.NOTIFICATION_TYPES).forEach(function (key) {

jsSdkUtils.objectValues(enums.NOTIFICATION_TYPES).forEach(function (notificationTypeEnum) {

mjc1283 · 2020-02-21T17:43:28Z

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

@@ -60,7 +60,8 @@ module.exports = {
    });

    projectConfig.rolloutIdMap = fns.keyBy(projectConfig.rollouts || [], 'id');
-    fns.forOwn(projectConfig.rolloutIdMap, function(rollout) {
+    Object.keys(projectConfig.rolloutIdMap || {}).forEach(function (rolloutId) {


Suggested change

Object.keys(projectConfig.rolloutIdMap || {}).forEach(function (rolloutId) {

jsSdkUtils.objectValues(projectConfig.rolloutIdMap || {}).forEach(function (rollout) {

mjc1283 · 2020-02-21T17:45:52Z

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

@@ -79,8 +80,8 @@ module.exports = {

      // Creates { <variationId>: { key: <variationKey>, id: <variationId> } } mapping for quick lookup
      fns.assignIn(projectConfig.variationIdMap, fns.keyBy(experiment.variations, 'id'));
-
-      fns.forOwn(experiment.variationKeyMap, function(variation) {
+      Object.keys(experiment.variationKeyMap || {}).forEach(function (variationKey) {


Suggested change

Object.keys(experiment.variationKeyMap || {}).forEach(function (variationKey) {

jsSdkUtils.objectValues(experiment.variationKeyMap || {}).forEach(function (variation) {

mjc1283 · 2020-02-21T17:46:30Z

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

@@ -92,7 +93,8 @@ module.exports = {
    projectConfig.experimentFeatureMap = {};

    projectConfig.featureKeyMap = fns.keyBy(projectConfig.featureFlags || [], 'key');
-    fns.forOwn(projectConfig.featureKeyMap, function(feature) {
+    Object.keys(projectConfig.featureKeyMap || {}).forEach(function (featureKey) {


Suggested change

Object.keys(projectConfig.featureKeyMap || {}).forEach(function (featureKey) {

jsSdkUtils.objectValues(projectConfig.featureKeyMap || {}).forEach(function (feature) {

mjc1283 · 2020-02-21T17:46:58Z

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

-        }
-      }.bind(this)
-    );
+    Object.keys(configObj.featureKeyMap).forEach(function (featureKey) {


Suggested change

Object.keys(configObj.featureKeyMap).forEach(function (featureKey) {

jsSdkUtils.objectValues(configObj.featureKeyMap).forEach(function (feature) {

mjc1283 · 2020-02-26T16:36:32Z

@zashraf1985 LGTM - could you please resolve the merge conflicts?

zashraf1985 · 2020-02-26T17:25:52Z

@mjc1283 this is ready to be merged

zashraf1985 added 2 commits February 17, 2020 22:22

replaced lodash.forOwn with native code

876227f

fixed lint errors

426feaf

zashraf1985 marked this pull request as ready for review February 18, 2020 06:38

zashraf1985 requested a review from a team as a code owner February 18, 2020 06:38

mjc1283 suggested changes Feb 18, 2020

View reviewed changes

optimizely-codereview-bot assigned zashraf1985 Feb 18, 2020

1. Added early break when callback returns false.

d0c510c

2. Added unit tests

zashraf1985 removed their assignment Feb 18, 2020

optimizely-codereview-bot requested a review from mjc1283 February 18, 2020 19:17

optimizely-codereview-bot added the 🤖 4 hrs label Feb 18, 2020

mjc1283 suggested changes Feb 18, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 4 hrs label Feb 18, 2020

optimizely-codereview-bot assigned zashraf1985 Feb 18, 2020

zashraf1985 and others added 2 commits February 20, 2020 19:26

Merge branch 'master' into zeeshan/replace-lodash-forown

b2a849d

removed fns.forOwn altogether and replaced all the usage instances wi…

d819482

…th Object.keys

zashraf1985 changed the title ~~Removed dependency on lodash.forOwn~~ refactor: Removed dependency on lodash.forOwn Feb 21, 2020

zashraf1985 removed their assignment Feb 21, 2020

optimizely-codereview-bot requested a review from mjc1283 February 21, 2020 07:06

mjc1283 suggested changes Feb 21, 2020

View reviewed changes

optimizely-codereview-bot assigned zashraf1985 Feb 21, 2020

zashraf1985 and others added 2 commits February 24, 2020 10:19

Merge branch 'master' into zeeshan/replace-lodash-forown

a232b38

used objectValues where applicable

6371783

zashraf1985 removed their assignment Feb 24, 2020

optimizely-codereview-bot requested a review from mjc1283 February 24, 2020 20:36

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs and removed 🤖 4 hrs labels Feb 25, 2020

zashraf1985 added 2 commits February 25, 2020 13:11

Merge branch 'master' into zeeshan/replace-lodash-forown

247f088

Merge branch 'master' into zeeshan/replace-lodash-forown

9d3a331

optimizely-codereview-bot added 🤖 12 hrs and removed 🤖 8 hrs labels Feb 26, 2020

mjc1283 approved these changes Feb 26, 2020

View reviewed changes

zashraf1985 and others added 2 commits February 26, 2020 09:08

Merge branch 'master' into zeeshan/replace-lodash-forown

08cb4bf

fixed lint issue

721aef6

mjc1283 merged commit e07181e into master Feb 27, 2020

mjc1283 deleted the zeeshan/replace-lodash-forown branch February 27, 2020 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Removed dependency on lodash.forOwn #399

refactor: Removed dependency on lodash.forOwn #399

Uh oh!

zashraf1985 commented Feb 18, 2020 •

edited

Loading

Uh oh!

coveralls commented Feb 18, 2020 •

edited

Loading

Uh oh!

mjc1283 Feb 18, 2020

Uh oh!

zashraf1985 Feb 18, 2020

Uh oh!

mjc1283 left a comment

Uh oh!

mjc1283 Feb 21, 2020

Uh oh!

mjc1283 Feb 21, 2020

Uh oh!

mjc1283 Feb 21, 2020

Uh oh!

mjc1283 Feb 21, 2020

Uh oh!

mjc1283 Feb 21, 2020

Uh oh!

mjc1283 Feb 21, 2020

Uh oh!

mjc1283 Feb 21, 2020

Uh oh!

mjc1283 commented Feb 26, 2020

Uh oh!

zashraf1985 commented Feb 26, 2020

Uh oh!

Uh oh!

	Object.keys(enums.NOTIFICATION_TYPES).forEach(function(key) {
	jsSdkUtils.objectValues(enums.NOTIFICATION_TYPES).forEach(function(notificationTypeEnum) {

	Object.keys(projectConfig.rolloutIdMap \|\| {}).forEach(function (rolloutId) {
	jsSdkUtils.objectValues(projectConfig.rolloutIdMap \|\| {}).forEach(function (rollout) {

	Object.keys(experiment.variationKeyMap \|\| {}).forEach(function (variationKey) {
	jsSdkUtils.objectValues(experiment.variationKeyMap \|\| {}).forEach(function (variation) {

	Object.keys(projectConfig.featureKeyMap \|\| {}).forEach(function (featureKey) {
	jsSdkUtils.objectValues(projectConfig.featureKeyMap \|\| {}).forEach(function (feature) {

	Object.keys(configObj.featureKeyMap).forEach(function (featureKey) {
	jsSdkUtils.objectValues(configObj.featureKeyMap).forEach(function (feature) {

refactor: Removed dependency on lodash.forOwn #399

refactor: Removed dependency on lodash.forOwn #399

Uh oh!

Conversation

zashraf1985 commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

coveralls commented Feb 18, 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!

mjc1283 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!

mjc1283 commented Feb 26, 2020

Uh oh!

zashraf1985 commented Feb 26, 2020

Uh oh!

Uh oh!

zashraf1985 commented Feb 18, 2020 •

edited

Loading

coveralls commented Feb 18, 2020 •

edited

Loading