Treat empty objects as null

[mhuebert]

Updates Targaryen to treat empty objects the same as null (as Firebase does), with tests.

[mhuebert]

Great, thanks!

But, I just realized that there's no obviously correct behaviour here. I was thinking of Firebase's set command, for which a root-level {} and null are equivalent, but there is also .update(), which performs a shallow merge, so {} is a no-op at the root level, and only equivalent to null at depth>0. (But update also supports multi-segment paths, so you get the equivalent of a deep merge by supplying paths as keys, eg/ {"a/b/c": 1, "a/b/d": 2}, which are forbidden in .set()).

It would involve extending the API, but I could work on adding an argument to merge to specify set/update behaviour that matches FB?

[dinoboff]

I am not sure I follow about the issue with update.

An update (Ruleset.prototype.tryPatch) doesn't merge the new data, it iterate other each key, and merge each value (which maybe a bug btw - see #52, for both tryPatch and tryWrite); an empty object would get merge, nothing would happen.

[mhuebert]

Hmm. I think for tryWrite to be consistent with set and tryPatch with update, tryWrite would not merge values at all (only .priority), and tryPatch would not merge values (except for .priority) that exist at the 'endpoint' of a path. eg

tryWrite... old data: {'b', 1},  new data: {'a', 1},
            => {'a', 1}

tryWrite... old data: {'b', 1}, new data: {}, 
            => null

tryPatch... old data: {'b', 1}, new data: {'a', 1}, 
            => {'a', 1, 'b', 1}

tryPatch... old data: {'a': {'b': {'c': 1}}}, new data: {'a/b': {'d': 1}}, 
            => {'a': {'b': {'d': 1}}}

tryPatch... old data: {'a': {'b': {'c': 1}}}, new data: {'a/b': {}}, 
            => null

[dinoboff]

Why do you need to check the constructor?

[mhuebert]

per here, a new Date() would also pass as an empty object without the extra check.

[dinoboff]

I fixed the formatting issue and added a test to make sure update operations (tryPatch) are not affected.

@mhuebert can you please review and it would work for your use cases?

If you are looking for alternative, you could leave the underlying data intact (keep setting nodes to empty objects) and update RuleDataSnapshot.prototype.val() to return null if the value is an empty object.

[mhuebert]

Thanks for the formatting fix. I've added tests for some additional behaviour to align with set/update. Four are currently failing, I thought it might be useful to have them reviewed to make sure functionality makes sense.

The two main issues -

1 - set (tryWrite) operations should replace, rather than merge with, existing data (except for .priority, which is retained unless specifically overwritten). This is also true for each target path of an update (tryPatch) operation.
2 - null/empty children should be pruned in order for .val() and .exists() to behave correctly. Keys of an object whose values are set to null should be removed from the object, and a null value essentially propagates upwards until it finds a non-null sibling, ie. {a: 1, b: {c: {d: null}}} becomes {a: 1}.

I wrote some exploratory code earlier this week that resolves some of this and noticed that fixing it breaks some earlier tests, which expect the current merge and null behaviour.

[dinoboff]

@mhuebert I don't think it should be part of this PR. See PR #52 for that.

[dinoboff]

I merged the changes without tests related to #52. Please Open a new PR with the new tests.

[mhuebert]

Looks great. With all of today's merges, all of my tests pass except null handling, which I will work on as a separate issue. Thanks!