deepComputedFrom breaks when object contains a reference loop

[avrahamcool]

consider the following Node model, that represent a node in a Doubly linked list. the combined getter should list all the values starting the root, to the last child.

export interface Node
{
  parent?: Node;
  child?: Node;
  value?: string;
}

export class App
{
  root: Node;
  constructor()
  {
    this.root = {};

    this.root.child = {
      parent: this.root
      }
  }

  @deepComputedFrom("root")
  get combined(): string
  {
    let root = this.root;
    let value = "";
    while(root)
    {
      value += `${root.value} =>`;
      root = root.child;
    }
    return value;
  }
}

but: we get an error - because there is a reference loop between the parent and the child.

Uncaught (in promise) RangeError: Maximum call stack size exceeded
    at Function.[Symbol.hasInstance] (<anonymous>)
    at getDependency (index.js?3ae2:309)
    at ObjectPropertyDependency.collect (index.js?3ae2:91)
    at eval (index.js?3ae2:59)
    at Array.forEach (<anonymous>)
    at ObjectDependency.collect (index.js?3ae2:56)
    at ObjectPropertyDependency.collect (index.js?3ae2:96)
    at eval (index.js?3ae2:59)
    at Array.forEach (<anonymous>)
    at ObjectDependency.collect (index.js?3ae2:56)

this can be fixed by marking the visited objects after they have been "patched". or saving a Set with all "patched" objects. that way: the process of deep traversing will not run into endless loops.

[avrahamcool]

I'm trying to create a PR for this, but it turn-out to be more difficult than I initially thought.

at first I thought it's enough to fix the ObjectDependency.collect, but now I realize we need to fix all collect scenarios. an object can be a part of the loop in many ways.

• the obvious way - (x => y => x) like in the issue above
• as part of an arbitrary length loop (a => b => c => ... => a)
• as part of an internal collection of some sort (a => b[7] => a) etc.

because I don't want to add a flag directly on the current observed object. (maybe we can use metadata API?), I need to keep track of all "items" that are already collected. - regardless of where they are in the actual object.

you already have an owner object that gets passed down the hirearchy. can I add a Set there? to keep track of every visited item?

its sound simple, but I don't have the full picture of how things works in mind. would I need to sync this set when new objects gets added after initial collection? would I need to remove them from the set when they are disconnected from the original object? etc.

I'd like your input on that.

or even better - if you are already working on a fix for that. :) that will be great.

[bigopon]

you already have an owner object that gets passed down the hirearchy.

This is what im thinking too. Can use a weakmap to associate the owner object with a set of tracked object.

And no, i havent worked on a PR yet, anyncontribution will be really welcomed 😃

[bigopon]

From what you commented, I thi k you already got an idea of how it all shouls work together 👍

[avrahamcool]

well, I've tried to implement that in a fork. it's seems to work only if I had all of the property that I was intending to change present in my VM beforehand.

for example: I had to explicitly set the value property:

constructor() {
  this.root = {value: undefined};
  this.root.child = {parent: this.root, value: undefined};
}

I think it it was always like that, and not because of my changes. (am I right?)

if I tried like that - the changes would not fire:

constructor() {
  this.root = {};
  this.root.child = {parent: this.root};
}

in a real case scenario - when you have bindings from the view to that non-existing property - aurelia will create a property for it, so this will be unnoticeable.

overall, introducing the WeakMap did not break any other test.. except: it lowered the amount of times values were being re-evaluted. I didn't fix those tests - because I wanted your opinion on the all solution beforehand.

before inserrting things into the WeakMap, I had to make sure they are objects. I think that could have been done nicely with a helper function, instead of repeating the same code over and over. I'll do in the offical PR that if this a valid solution.

[bigopon]

@avrahamcool that sounds cool, let's create a PR 🚀 For the predefined property issue, it's kind of a requirement, as it's impossible to do without it, unless we use proxy. I think it's ok to not involve proxy at all in v1, so that we can keep it compat to older JS environments.

[avrahamcool]

the reop contains dist files, what is the policy about the PR? should it update the dist? or just src?

[bigopon]

Just the src, I'll do the publish after the merge.

[avrahamcool]

I was wrong. my simple implementation doe's break some test. (I was able to see that only after I cleaned the json counts errors). although its sounds good, and should work in theory.

I'll try to dive in more deeply in the upcoming.