Failing code after recent changes to object cloning

TW80000 commented 5 years ago

Context

Changes were made recently to the cloning method for plain JavaScript objects such that they do a true deep clone instead of simply returning a reference to the argument.

Stack trace of failing tests

Failures:
1) foam.Object clone
  Message:
    Expected Object({ d: 'hello' }) to be Object({ d: 'hello' }).
  Stack:
    Error: Expected Object({ d: 'hello' }) to be Object({ d: 'hello' }).
        at UserContext.<anonymous> (/home/travis/build/foam-framework/foam2/test/src/core/stdlib.js:844:34)
2) foam.util handles a buffet of types
  Message:
    Expected false to be true.
  Stack:
    Error: Expected false to be true.
        at /home/travis/build/foam-framework/foam2/test/src/core/stdlib.js:979:55
        at Array.forEach (native)
        at UserContext.<anonymous> (/home/travis/build/foam-framework/foam2/test/src/core/stdlib.js:974:11)
3) DirTreeHandler should serve file based on path prefix
  Message:
    RangeError: Maximum call stack size exceeded
  Stack:
    RangeError: Maximum call stack size exceeded
        at Object.isInstance (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:698:39)
        at typeOf (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:842:16)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:933:39)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:790:35)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:933:49)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:790:35)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:933:49)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:790:35)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:933:49)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:790:35)
  Message:
    TypeError: Cannot read property 'then' of undefined
  Stack:
    TypeError: Cannot read property 'then' of undefined
        at UserContext.<anonymous> (/home/travis/build/foam-framework/foam2/test/src/net/node/DirTreeHandler.js:55:18)
4) DirTreeHandler should prohibit escape-by-relative-path
  Message:
    RangeError: Maximum call stack size exceeded
  Stack:
    RangeError: Maximum call stack size exceeded
        at Object.isInstance (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:698:39)
        at typeOf (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:842:16)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:933:39)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:790:35)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:933:49)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:790:35)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:933:49)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:790:35)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:933:49)
        at Object.clone (/home/travis/build/foam-framework/foam2/src/foam/core/stdlib.js:790:35)
  Message:
    TypeError: Cannot read property 'then' of undefined
  Stack:
    TypeError: Cannot read property 'then' of undefined
        at UserContext.<anonymous> (/home/travis/build/foam-framework/foam2/test/src/net/node/DirTreeHandler.js:77:18)

Problems

Deep cloning fails when the object being cloned is huge.
- For example, the 3rd and 4th failing tests in the stack trace above failed because some code tried to clone a DirTreeHandler, which has a property named path that was set to the entire Node.js "path" library. Since FOAM recognized the path library as an object, the clone method tried to deep clone it, which lead to a "Maximum call stack size exceeded" error.
Deep cloning is slower than shallow cloning

Possible solutions

Add a `deepClone` method

Problem 2 paired with the fact that deep cloning is only strictly necessary some of the time means shallow cloning is a reasonable default because it will be faster. If shallow cloning isn't sufficient for the use case, a deep clone can be explicitly performed instead. Therefore this solution is to add a separate deepClone method that can be called when deep cloning is required, and all other cases can just call clone.

One advantage of this solution is that it doesn't require any changes to existing code.

Add a `shallowClone` method

Same as above, but the opposite. Add a method that callers can use to explicitly request a shallow clone to avoid problems like Problem 1. The clone method remains a deep clone.

This requires us to go back on a case by case basis and change existing code that broke because of the change from clone being shallow to deep. It can be fixed by calling the new shallowClone method instead.

TW80000 commented 5 years ago

@lchanmann @kgrgreer @jlhughes @mcarcaso @adamvy

lchanmann commented 5 years ago

@TW80000 do we have progress on this issue?

TW80000 commented 5 years ago

I haven't done any more work on it, but I did think of an even better 3rd solution.

Property already defines a cloneProperty function that properties can implement to provide custom deep cloning behaviour. We should just implement this method on special cases like properties that are MDAOs or node libraries and simply return a reference in those cases rather than doing the default behaviour of deep cloning.

https://github.com/foam-framework/foam2/blob/master/src/foam/core/Property.js#L144-L153

What do you think of that? @lchanmann

foam-framework / foam2