anvaka / ngraph.merge

Simple merge utility to extend objects without extra dependencies
MIT License
2 stars 4 forks source link

Stack overflow issue #1

Open rafaeltp opened 9 years ago

rafaeltp commented 9 years ago

First, just to give a bit of history: I notice the problem while trying to merge two objects with containers in it. The problem is that container have fields to both parents and children, which cause an infinity loop.

To keep it simple, but trying to reproduce the problem with accuracy, I re-wrote the goes deep test:

test('goes deep', function(t) {
  var dad = { age: 42, name: 'John' };
  var kid = { age: 15, name: 'Chuck', father: dad };

  dad["kids"] = [ kid ];

  var otherkid = { age: 13, name: 'Allyson', father: dad };
  merge(dad, { last_name: "Smith", kids: [otherkid] }); 

  t.equals(dad.age, 42);
  t.equals(dad.last_name, 'Smith');
  t.equals(dad.kids[0].name, 'Chuck');
  t.equals(dad.kids[1].name, 'Allyson');
  t.end();
});

Which gives me the following error:

    not ok 5 RangeError: Maximum call stack size exceeded
      ---
        type:    RangeError
        message: Maximum call stack size exceeded
        code:    stack_overflow
        errno:   ~
        stack:   
          - 
        thrown:  true
      ...

This is consistent with original issue.

PS: I'm not experienced in JavaScript, so I may not be doing some coding in an optimal way.

anvaka commented 9 years ago

Interesting! I see couple problems with current implementation of the ngraph.merge, but that was intentional, as the old comment is saying:

// go deep, don't care about loops here, we are simple API!

I wonder maybe ngraph.merge isn't the right library for your task? How are you going to merge arrays? I see in your test you expect to have two elements in the array, but what if both elements at index 0 have the same name - should they be merged to one?