multiple parents and node invariant check

[fundkis]

Hi, First, we want to thank you for this library. We loved it and we loved the way it simplify react interaction... We found recently an issue in our code that makes the freezer nodes point to multiple parents (old node pointers) that makes the whole freezer tree incoherent: children pointing to parents not on the freezer tree. The issue is our fault, but it was very hard to detect. It could have been much easier if freezer integrate some check code that can be enabled in debug... First, we wrote an invariant function that verifies the whole tree. This is how we figured out that there was a bug. When debugging we realized that freezer allows multiple parents for a single node. We don't use this feature and we consider it bad usage in general to have such a tree. If we disable such a feature, we can enforce that any subtree that need to be plugged in a feezer tree should not be frozen (should not contain the __ field). We implemented this precondition check and this is what help us to identify our guilty code. To sum up, freezer should provide :

• some debug code to check for some invariants and preconditions
• allow for a "signleParent" option that allows for more fine grained preconditions

If you agree with the above and you are ready to integrate our contribution, we will be happy to provide a Pull Request. Otherwise, we think about a lighter implementation that will be compatible with a subset of freezer-js...

[arqex]

Hi @fundkis

Thanks for your thoughts, you have described the most usual problem when working with freezer.js, using a node that has been detached from the store. I completely agree that it is a problem very difficult to debug, and I have learnt to use freezer in a way that minimize the chances of getting an incoherent state like the one you describe, but I can imagine that many people suffering this issue. I will be glad to study and merge your solution.

As for the singleParent checking, and the need of not having the nodes frozen, I think I didn't understand it well, can you explain it further? The immutability of the nodes are a nice feature to make sure that the tree is updated the right way, but it's possible to disable it using mutable: true in the initialization. https://github.com/arqex/freezer#api

Also letting a node be in 2 different places in the tree is a feature, but I am not against creating an option to enforce the opposite if it's not making the library much more complex and can help to detect errors.

Thanks again for your thoughts they are really valuable :)

[fundkis]

thank your for the answer. I've seen that from the tests that "letting one node to be in different places" is a feature even if it is not a documented one. We don't use that feature and we think that it is in general a bad practice: we like streamable state (the one you could rebuild with a JSON.parse(JSON.stringify(state)). if we add a singleParent option that disable this feature, then we can forbid completely to add a frozen node in the tree. For instance we can check that the whole attrs argument of the merge function (in frozen.js) does not contain any frozen node. We can then assert as a precondition on main functions that only plain js objects can "enter" the library. This allows to detect any misuse very early. We can certainly do without this option: the precondition may be that you can pass only nodes that are already part of the tree and having a verification code enforcing that on debug, but this is not what we've done...

[arqex]

Now I see, you meant that with the 'singleParent' option set to true, no frozen nodes could be added to the tree. Sounds fair enough to add the option, but its default value would be false to not to break any current installation.

[fundkis]

of course. we should be backward compatible... I will also disable by default, the whole debugging checks and even skip the whole debugging code from the bundled js file. I'll try to submit a PR by the end of the week