Repository: ViacomInc/data-point (js)

Issue:
implement isPlainObject on ConstantReducer
<!--
Thanks for your interest in the project. We appreciate bugs filed and PRs submitted!
Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Please fill/un-comment out this template with all the relevant information so we can
understand what's going on and fix the issue.
-->

**Problem description**:

https://github.com/ViacomInc/data-point/pull/383#discussion_r316423143


**Suggested solution**:

On ConstantReducer.resolve only execute cloneDeep if its a plain object, meaning move the isPlainObject out of the existing module to a place where its common and can be used by both.

Pull request description:
 constant check object
<!--
Thanks for your interest in the project. We appreciate bugs filed and PRs submitted!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!

BREAKING CHANGES:

If your PR includes a breaking change, please submit it with a codemod under
data-point-codemods that will help users upgrade their codebase.

Breaking changes without a codemod will not be accepted unless a codemod is not
viable or does not apply to the specific situation.
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->
**What**:

closes #386 

<!-- Why are these changes necessary? -->
**Why**:

As per discusses in https://github.com/ViacomInc/data-point/pull/383#discussion_r316423143

<!-- Have you done all of these things?  -->
**Checklist**:
<!-- add "N/A" to the end of each line that's irrelevant to your changes -->
<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->
- [ ] Has Breaking changes
- [ ] Documentation
- [X] Tests
- [X] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [ ] Added username to **all-contributors** list

<!-- feel free to add additional comments -->

Patch:
diff --git a/packages/data-point/src/ReducerObject.js b/packages/data-point/src/ReducerObject.js
index 9878c94..cc6e76d 100644
--- a/packages/data-point/src/ReducerObject.js
+++ b/packages/data-point/src/ReducerObject.js
@@ -3,12 +3,7 @@ const cloneDeep = require("lodash/cloneDeep");
 
 const { Reducer } = require("./Reducer");
 const isReducerConstant = require("./is-reducer-constant");
-
-function isPlainObject(obj) {
-  return (
-    typeof obj === "object" && Object.getPrototypeOf(obj) === Object.prototype
-  );
-}
+const isPlainObject = require("./is-plain-object");
 
 function newProps() {
   return {
diff --git a/packages/data-point/src/is-plain-object.js b/packages/data-point/src/is-plain-object.js
new file mode 100644
index 0000000..dbc99bc
--- /dev/null
+++ b/packages/data-point/src/is-plain-object.js
@@ -0,0 +1,6 @@
+function isPlainObject(obj) {
+  return (
+    typeof obj === "object" && Object.getPrototypeOf(obj) === Object.prototype
+  );
+}
+module.exports = isPlainObject;
diff --git a/packages/data-point/src/reducer-helpers/constant/constant.js b/packages/data-point/src/reducer-helpers/constant/constant.js
index 9651262..7688752 100644
--- a/packages/data-point/src/reducer-helpers/constant/constant.js
+++ b/packages/data-point/src/reducer-helpers/constant/constant.js
@@ -1,14 +1,24 @@
 const cloneDeep = require("lodash/cloneDeep");
 
 const { Reducer } = require("../../Reducer");
+const isPlainObject = require("../../is-plain-object");
+
+/**
+ * @param {Boolean} isObject true if parameter `value` is assumed to be an object
+ * @param {any} value
+ */
+function cloneObject(isObject, value) {
+  return isObject ? cloneDeep(value) : value;
+}
 
 class ReducerConstant extends Reducer {
   constructor(spec) {
     super("constant", undefined, spec);
+    this.isPlainObject = isPlainObject(spec);
     // We clone the source to truly respect the nature of a constant, if the
     // original source is ever changed the data integrity of the constant
     // should be kept
-    this.constantValue = cloneDeep(spec);
+    this.constantValue = cloneObject(this.isPlainObject, spec);
   }
 
   static create(spec) {
@@ -21,10 +31,11 @@ class ReducerConstant extends Reducer {
   resolve() {
     // this clone happens so any future changes to the output do not
     // affect the original value of this.constantValue
-    return cloneDeep(this.constantValue);
+    return cloneObject(this.isPlainObject, this.constantValue);
   }
 }
 
 module.exports = {
+  cloneObject,
   ReducerConstant
 };


Test patch:
diff --git a/packages/data-point/src/ReducerObject.test.js b/packages/data-point/src/ReducerObject.test.js
index 212b712..4c0f479 100644
--- a/packages/data-point/src/ReducerObject.test.js
+++ b/packages/data-point/src/ReducerObject.test.js
@@ -15,19 +15,6 @@ afterEach(() => {
   jest.clearAllMocks();
 });
 
-describe("isPlainObject", () => {
-  it("should return true for plain objects", () => {
-    expect(reducerObject.isPlainObject({})).toEqual(true);
-    expect(reducerObject.isPlainObject("string")).toEqual(false);
-    expect(reducerObject.isPlainObject(123)).toEqual(false);
-    expect(reducerObject.isPlainObject(() => true)).toEqual(false);
-
-    expect(reducerObject.isPlainObject(Object.create({}))).toEqual(false);
-    class Test {}
-    expect(reducerObject.isPlainObject(new Test())).toEqual(false);
-  });
-});
-
 describe("getObjectProperties", () => {
   it("should extract all constant", () => {
     const obj = {
diff --git a/packages/data-point/src/is-plain-object.test.js b/packages/data-point/src/is-plain-object.test.js
new file mode 100644
index 0000000..6c113ed
--- /dev/null
+++ b/packages/data-point/src/is-plain-object.test.js
@@ -0,0 +1,14 @@
+const isPlainObject = require("./is-plain-object");
+
+describe("isPlainObject", () => {
+  it("should return true for plain objects", () => {
+    expect(isPlainObject({})).toEqual(true);
+    expect(isPlainObject("string")).toEqual(false);
+    expect(isPlainObject(123)).toEqual(false);
+    expect(isPlainObject(() => true)).toEqual(false);
+
+    expect(isPlainObject(Object.create({}))).toEqual(false);
+    class Test {}
+    expect(isPlainObject(new Test())).toEqual(false);
+  });
+});
diff --git a/packages/data-point/src/reducer-helpers/constant/constant.test.js b/packages/data-point/src/reducer-helpers/constant/constant.test.js
index 311d020..c47266e 100644
--- a/packages/data-point/src/reducer-helpers/constant/constant.test.js
+++ b/packages/data-point/src/reducer-helpers/constant/constant.test.js
@@ -1,4 +1,29 @@
-const { ReducerConstant } = require("./constant");
+jest.mock("lodash/cloneDeep");
+const mockCloneDeep = require("lodash/cloneDeep");
+
+mockCloneDeep.mockImplementation(jest.requireActual("lodash/cloneDeep"));
+
+const { ReducerConstant, cloneObject } = require("./constant");
+
+afterEach(() => {
+  jest.clearAllMocks();
+});
+
+describe("cloneObject", () => {
+  it("should not clone if parameter isObject is passed as false", () => {
+    const result = cloneObject(false, "string");
+    expect(mockCloneDeep).not.toBeCalled();
+    expect(result).toEqual("string");
+  });
+
+  it("should clone if parameter isObject is passed as true", () => {
+    const source = {};
+    const result = cloneObject(true, source);
+    expect(mockCloneDeep).toBeCalled();
+    source.mutated = true;
+    expect(result).toEqual({});
+  });
+});
 
 describe("ReducerConstant", () => {
   describe("constructor", () => {
@@ -12,6 +37,13 @@ describe("ReducerConstant", () => {
       expect(reducer.constantValue).toEqual("value");
     });
 
+    it("should not clone non-plain objects", () => {
+      const reducer = new ReducerConstant(true);
+
+      expect(reducer.constantValue).toEqual(true);
+      expect(mockCloneDeep).not.toBeCalled();
+    });
+
     it("should set constantValue to be deep cloned from source received", () => {
       const sourceObject = {
         test: "test"
@@ -44,7 +76,7 @@ describe("ReducerConstant", () => {
       expect(reducer.resolve()).toEqual(sourceObject);
     });
 
-    it("should return deep cloned value", () => {
+    it("should return deep cloned value for plain objects", () => {
       const sourceObject = {
         test: "test"
       };
@@ -57,5 +89,12 @@ describe("ReducerConstant", () => {
         test: "test"
       });
     });
+
+    it("should return value for non-plain objects", () => {
+      const reducer = ReducerConstant.create("string");
+      const result = reducer.resolve();
+      expect(result).toEqual("string");
+      expect(mockCloneDeep).not.toBeCalled();
+    });
   });
 });