feat: improve deep merge function #376

blackswanny · 2018-10-11T00:32:55Z

improve deep merge function to avoid stack overflow and possible circular dependency

schnerd · 2018-10-11T02:08:50Z

src/utils/__tests__/deep-merge.test.js

@@ -17,4 +24,26 @@ describe('deepMerge', () => {
    const ret = deepMerge({}, target, source1, source2);
    expect(ret).toEqual({foo: 3, bar: 2, baz: 3, quux: 4});
  });
+
+  test.only('should forcefully merge very right source for heavy objects to avoid stack overflow and possible circular dependency', () => {


remove test.only?

schnerd · 2018-10-11T02:10:21Z

src/utils/deep-merge.js

@@ -25,5 +35,5 @@ export default function deepMerge(
 }

 function isCloneable(obj: mixed) {
-  return Array.isArray(obj) || {}.toString.call(obj) == '[object Object]';
+  return typeof obj === 'object' || typeof obj === 'function';


why typeof obj === 'function'? seems like you wouldn't want to call deepMerge with a function as one of the arguments.

may be I didn't get it, but actually function is object and I want it to be called in deepMerge as a normal object

if you check where isClonable is used, it's doing this:

if (isCloneable(value)) { target[key] = _deepMerge.apply(this, [target[key] || {}, value]) }

Imagine that value is a function like () => true, this means you're trying to make a call like:

deepMerge(obj, () => true)

this makes no sense. you should only be passing object/null/undefined to deepMerge.

What was wrong with the initial implementation of this method? It's what was used in just-extend which is battle tested in countless production apps. We should just keep that.

nadiia · 2018-10-11T02:14:23Z

src/utils/deep-merge.js

@@ -25,5 +35,5 @@ export default function deepMerge(
 }

 function isCloneable(obj: mixed) {
-  return Array.isArray(obj) || {}.toString.call(obj) == '[object Object]';
+  return typeof obj === 'object' || typeof obj === 'function';


Why do you consider functions and null cloneable?

i did exclude null, but function is an object. Lots of frameworks starting with jQuery use it as object with props, especially it's common if it's a function-constructor

Could you point out where you exclude the null? How are you intended to deepMerge functions?

Just pushed fresh code. Function merges the same way as any other object. We go through it's props and add them to target function, which is the prop itself of parent object

nadiia · 2018-10-11T02:15:45Z

src/utils/deep-merge.js

-  target?: ?{},
-  ...sources: Array<null | ?{}>
-): {} {
+const MAX_DEPTH_OF_CHECK = 10;


Where the number 10 comes from?

we can replace it with any other likable number. When I did research about redux data-objects in some project, the most optimal size was 6, deeper there was "trash" that sometimes appears in data-object inaccurately mixed in. Other number is 40 as it's a const for stack overflow in JS runtime. I can put here any reasonable number if any

nadiia

Left a couple comments to address

gergelyke · 2018-10-11T15:58:21Z

can we simply use https://www.npmjs.com/package/lodash.merge or https://www.npmjs.com/package/just-extend instead of managing our own?

blackswanny · 2018-10-11T17:32:30Z

@gergelyke this PR is only to improve it, the method was added long before. However, to compare
lodash.merge - is smarter as it counts some specifics of property descriptor, but it is times more heavy by package size, in very basics it uses the same condition value != null && (type == 'object' || type == 'function'); as we use here

just.extend is lighter, but still contains many code, which we don't need, whereas we need only extend function, which is almost the same as ours (seems to be previous version of it)
https://github.com/angus-c/just/blob/master/packages/object-extend/index.js

gergelyke · 2018-10-11T17:34:43Z

Yeah, I know it's been there - my question is if it is worth maintaining it, while we could just pull something on.

blackswanny · 2018-10-11T17:40:27Z

@gergelyke that depends on strategy we follow. If we try to avoid extra dependencies, and take into account those drawbacks I mentioned, than we should maintain our own.

gergelyke · 2018-10-11T17:44:29Z

what are those drawbacks? how many KBs of extra we are talking about in comparison to out implementation?

blackswanny · 2018-10-11T18:34:14Z

@gergelyke size increased by 50KB. For others need more analysis, but this merge may be slower by performance due to more complex logic. Our implementation is 2KB size

gergelyke · 2018-10-11T18:36:11Z

Can you help me understand how could the size increase by 50Ks, if the whole file of just-extend is 1.5K?

nadiia · 2018-10-11T18:42:33Z

We can uses just-clone by @angus-c It's extremely small and has no dependencies!

blackswanny · 2018-10-11T18:43:17Z

@gergelyke this is for lodash.merge size.
just.extend is only 2KB in webpack. However it misses some smart checks and literally blindly extends, not merges

angus-c · 2018-10-11T18:45:04Z

@blackswanny what do you mean by blindly extends ? (also use just-clone as @nadiia says, it is even smaller)

blackswanny · 2018-10-11T18:55:52Z

@angus-c lodash has some safe way of merging. For instance, it checks if objects are the same and does not proceed. Also in this version we added stack overflow prevention, which might be common case in misuse of redux or other data frameworks.
just-clone may work, but It seems like it doesn't clone functions as objects, also it takes time to clone it, whereas we just need to merge\assign the very right source to target.

angus-c · 2018-10-11T19:40:17Z

@blackswanny I don't see lodash merge doing that check

const a = b = {a: 3};
a === b; // true
_.merge(a, b); // {a: 3};

it also doesn't prevent property clobbering

_.merge({a: 3}, {a: 4, b: 2}); // {a: 4, b: 2};

It does prevent circular reference errors although e.g. the following example also works in just-extend (maybe some circular references don't)

const extend = require('just-extend')
const x = {a: 2};
const y = {b: 3};
y.f = x;

extend(x, y);

Full tradeoffs are here https://github.com/angus-c/just/blob/master/TRADEOFFS.md

schnerd · 2018-10-11T20:31:42Z

I added this deepMerge implementation and basically copied the code directly out of just-extend as you can probably tell. I copied because it's just a simple function and I didn't want to add a dependency, but I'm indifferent to whether or not we use a dependency instead.

Regarding just-clone, maybe I'm missing something but I don't get how that helps here. We need to be able to deep merge two objects, it seems like just-clone is for cloning objects, not merging them.

While having protection against stack overflow due to cyclical references is nice, I'm wondering if we need to add it immediately–maybe we should wait until a customer actually runs into this issue and reports it. Also, if we flow type overrides and other inputs well, hopefully this should catch any cases where a user would be passing something cyclical.

blackswanny · 2018-10-11T20:43:32Z

@angus-c I just debugged this example and if a===b it comes back, which is safe fallback

for circular dependencies the next example causes stack overflow. Lodash has smarter way to prevent it, but it's more complex.

  const b = {};
  const c = {d: b};
  b.a = c;
  extend(true, b, c);

@schnerd yes, I may exclude it, but may at least leave error, so customer will find the bug easier. However, i remember this is pretty common case.

angus-c · 2018-10-11T20:52:18Z

@blackswanny Ok, I can add a patch for circular dependency to just-extend for that over the weekend. I couldn't reproduce the object equality check in lodash.merge

@schnerd BTW just-extend is 369 bytes bundled and minified so I don't think rolling our own will help much with size :) https://bundlephobia.com/result?p=just-extend@3.0.0

nadiia · 2018-10-11T20:56:03Z

I added this deepMerge implementation and basically copied the code directly out of just-extend as you can probably tell. I copied because it's just a simple function and I didn't want to add a dependency, but I'm indifferent to whether or not we use a dependency instead.

Regarding just-clone, maybe I'm missing something but I don't get how that helps here. We need to be able to deep merge two objects, it seems like just-clone is for cloning objects, not merging them.

While having protection against stack overflow due to cyclical references is nice, I'm wondering if we need to add it immediately–maybe we should wait until a customer actually runs into this issue and reports it. Also, if we flow type overrides and other inputs well, hopefully this should catch any cases where a user would be passing something cyclical.

My bad about mentioning the just-clone here, got lost in the bunch of comments and arguments
My biggest concern here as I mentioned earlier in several of my comments is merging functions 😳

blackswanny · 2018-10-11T21:37:53Z

@schnerd @nadiia Here is example. Old merge and new merge. As you see. Property 'bar' is not saved in old version as all props are eliminated by new function. We may consider that it's ok to override all props of function, but I am not sure that it's true for all frameworks and object using function as high-order objects

    const A = () => {};
    A.bar = 'bar';
    const B = () => {};
    B.foo = 'foo';
    const merged = deepMerge({override: A}, {override: B});

schnerd · 2018-10-11T22:27:50Z

@blackswanny For your example, i'd argue that the new merge semantics in this PR are incorrect. If A and B are components, you would want the deepMerge to return B, not A with the properties of B.

While you can assign properties to a function in javascript, I'm not sure that means we have to treat it like an object. This deepMerge helper should only be concerned with merging POJOs.

blackswanny · 2018-10-11T22:32:00Z

@schnerd I just fixed that. Now it returns B. However, we do have such examples, for instance all components in React are functions.

blackswanny · 2018-10-11T22:45:19Z

I removed check for functions

blackswanny · 2018-10-15T17:29:30Z

@schnerd yes, we can still use below code for custom height

        overrides={{
          DropDown: {
            style: {
              maxHeight: '100px',
            }
          }
        }}

blackswanny added the enhancement label Oct 11, 2018

blackswanny requested a review from schnerd October 11, 2018 00:33

blackswanny added the feature label Oct 11, 2018

schnerd reviewed Oct 11, 2018

View reviewed changes

nadiia reviewed Oct 11, 2018

View reviewed changes

nadiia suggested changes Oct 11, 2018

View reviewed changes

blackswanny force-pushed the deep-merge-improve branch from 7859e05 to 3341150 Compare October 11, 2018 04:35

blackswanny force-pushed the deep-merge-improve branch from 3341150 to 7ec58b5 Compare October 11, 2018 17:31

blackswanny force-pushed the deep-merge-improve branch from 7ec58b5 to d5e0106 Compare October 11, 2018 18:44

blackswanny force-pushed the deep-merge-improve branch 2 times, most recently from 40fc1b5 to d1c0635 Compare October 11, 2018 22:27

blackswanny force-pushed the deep-merge-improve branch from d1c0635 to 0e4418f Compare October 11, 2018 22:41

feat: improve deep merge function

862a2e4

blackswanny force-pushed the deep-merge-improve branch from 0e4418f to 862a2e4 Compare October 17, 2018 22:38

blackswanny closed this Oct 29, 2018

gergelyke deleted the deep-merge-improve branch November 13, 2018 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve deep merge function #376

feat: improve deep merge function #376

blackswanny commented Oct 11, 2018

schnerd Oct 11, 2018

blackswanny Oct 11, 2018

schnerd Oct 11, 2018

blackswanny Oct 11, 2018

schnerd Oct 11, 2018

nadiia Oct 11, 2018

blackswanny Oct 11, 2018

nadiia Oct 11, 2018

blackswanny Oct 11, 2018

nadiia Oct 11, 2018

blackswanny Oct 11, 2018

nadiia left a comment •

edited

gergelyke commented Oct 11, 2018

blackswanny commented Oct 11, 2018

gergelyke commented Oct 11, 2018

blackswanny commented Oct 11, 2018

gergelyke commented Oct 11, 2018

blackswanny commented Oct 11, 2018 •

edited

gergelyke commented Oct 11, 2018

nadiia commented Oct 11, 2018 •

edited

blackswanny commented Oct 11, 2018

angus-c commented Oct 11, 2018 •

edited

blackswanny commented Oct 11, 2018

angus-c commented Oct 11, 2018

schnerd commented Oct 11, 2018

blackswanny commented Oct 11, 2018

angus-c commented Oct 11, 2018

nadiia commented Oct 11, 2018 •

edited

blackswanny commented Oct 11, 2018 •

edited

schnerd commented Oct 11, 2018

blackswanny commented Oct 11, 2018

blackswanny commented Oct 11, 2018

blackswanny commented Oct 15, 2018

feat: improve deep merge function #376

feat: improve deep merge function #376

Conversation

blackswanny commented Oct 11, 2018

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

nadiia left a comment • edited

Choose a reason for hiding this comment

gergelyke commented Oct 11, 2018

blackswanny commented Oct 11, 2018

gergelyke commented Oct 11, 2018

blackswanny commented Oct 11, 2018

gergelyke commented Oct 11, 2018

blackswanny commented Oct 11, 2018 • edited

gergelyke commented Oct 11, 2018

nadiia commented Oct 11, 2018 • edited

blackswanny commented Oct 11, 2018

angus-c commented Oct 11, 2018 • edited

blackswanny commented Oct 11, 2018

angus-c commented Oct 11, 2018

schnerd commented Oct 11, 2018

blackswanny commented Oct 11, 2018

angus-c commented Oct 11, 2018

nadiia commented Oct 11, 2018 • edited

blackswanny commented Oct 11, 2018 • edited

schnerd commented Oct 11, 2018

blackswanny commented Oct 11, 2018

blackswanny commented Oct 11, 2018

blackswanny commented Oct 15, 2018

nadiia left a comment •

edited

blackswanny commented Oct 11, 2018 •

edited

nadiia commented Oct 11, 2018 •

edited

angus-c commented Oct 11, 2018 •

edited

nadiia commented Oct 11, 2018 •

edited

blackswanny commented Oct 11, 2018 •

edited