Avoid repeated tree serialization

[lightman76]

Caveat: I'm new to this library and the virtual-dom library, so might have missed something, but I think this is useful.

For my application, I'm going to be storing the json serialized diffs. When serializing a diff in JSON format, the full starting dom is included as "a" in the hash of the diff. Each VirtualPatch item has a reference to this tree called vNode (becomes v after transformation by this library). Serializing this portion of the tree is redundant since it's already included in this "a" key. This results in a lot of extra storage usage and extra serialization work.

This patch replaces the contents of the v key in the serialized format with a string of the form "i:" where "index num" is the same number used in as the key for the VirtualPatch in the diff hash (which appears to be the index into a depth first traversal of the full dom tree).

When deserializing, the "a" key is processed and a depth first traversal builds an array of the index numbers to the nodes in the tree. Then when processing the VirtualPatchS the v key is checked to see if it's a string starting with "i:" and if so, we look up the node for that index number and use that for vNode in the deserialized object.

To do this, I've added an optional ctx parameter to the toJson and fromJson. If not provided, it will be created. If ctx is being created and the object is a hash, it will test to see if it looks like a "diff" object and add the appropriate context parameters.

Please let me know if I've overlooked or misunderstood anything or you see any improvements that should be made.

[nolanlawson]

Thanks for the PR! Some questions though:

the v key is checked to see if it's a string starting with "i:"

So will this fail if the user has a span or some other element that contains i:?

I've added an optional ctx parameter

It would be nice to modify the README to add documentation as well :)

Also it appears to be failing on coverage, would appreciate getting coverage up to 100%. I haven't taken a deep look at the PR but would be happy to in a bit, thanks!

[lightman76]

Prior to my change, as I understand it, the v key was always a hash representing a vdom node, so there shouldn't be any ambiguity between the old node references and this string i:. The fact that the v key is a string should be enough of a check - i decided to add an i: prefix to the string in case there would ever be a need to use some other sort of way to reference the node (eg - by ids or something). If it's preferable, I could even make the v a number and drop the i prefix to save 4 characters per reference.

I'll see what's going on with the tests and fix up the coverage. I kept the ability to use the original v node format (where v is a hash) for compatibility in case people have the old format serialized somewhere, but since the tests now serialize in the new format, that path is not being tested. I'll add coverage for that.

[coveralls]

Coverage remained the same at 100.0% when pulling d3ef897 on lightman76:avoid_repeated_tree_serialization into cfd1409 on nolanlawson:master.

[lightman76]

Sorry for the delay on getting this done. Been swamped. Locally I'm getting 100% coverage, tests passing, and lint's ok.

As far as the optional ctx parameter, I can add something to the README about it if you want, but it's really not meant to be used externally. It's really for keeping track of things while recursively calling the methods. I guess if we wanted to keep that out of the public api, I could something like move all toJson/fromJson calls within the codebase to be toJsonInternal/fromJsonInternal and those would would have the ctx parameter.

[nolanlawson]

Great, if it's a hidden param, then I think it's fine to just not mention it in the README. Thanks for your dedication and for bringing this PR to completion! Tests passing and 100% coverage means it's a fairly easy choice. I would love to see a benchmark demonstrating the improvement, but given everything else you've done in this PR I'm happy to give the benefit of the doubt. Code looks good, thank you!

[nolanlawson]

published in 1.0.11