meteor/meteor — pull request #1734 by marcandre (2014-01-04 04:39:52+00:00)
Title: Ejson improvements

Hi!

When playing around with EJSON, I found a couple of things that I thought could be improved. This PR started quite small and grew bigger as I dug deeper. I didn't create individual PRs as the commits are sequential, I hope that's ok.

Here's a detailed discussion of each commit I've broken down this PR in 8 parts, so each can be discussed independently

0) No test
The first commit simply adds a basic test for custom types.

#### 1) Embedded custom types
The biggest issue is that the API for custom types requires JSON compatible data. Wouldn't it be best if EJSON compatible data could be used instead?

Taking for example a Person class with a 'date of birth' attribute (or an address that is from another custom type), then an implementer must take care of serializing it, e.g.:

People.prototype.toJSONValue = function() {
return {
name: this.name,
dob: EJSON.toJSONValue(this.dob), // this.dob would not work here
// etc...
}
}

I'm proposing a solution where a function `toEJSONValue` would be able to return an EJSON compatible value. My patch keeps compatibility with `toJSONValue` although I'm not sure how necessary this is. To do so, the factory argument of `addType` can be an object with a function `fromEJSONValue` instead of a factory.

See also [this stack overflow question](http://stackoverflow.com/questions/17598811/can-i-embed-an-ejson-object-inside-another-ejson-object)

#### 2) `typeName` is not DRY.
There are also very few times where I can imagine that the type returned depends on the object itself. If we allow for the function to be at the constructor level, then it's easier to both simplify the API and avoid the duplication of the custom type. Again, compatibility is maintained.

#### 3) Performance.
`EJSON.{to|from}JSONValue` (& thus `stringify`/`parse`) do a deep clone of the object. This means that simple objects, arrays, etc..., are duplicated, but this seems to be avoidable (partly or even completely in some cases like deserializing). This patch fixes that and no longer uses `EJSON.clone` for either serialization or deserialization. It also simplifies the codebase.

#### 4) support for `undefined`
I couldn't see a reason for `undefined` to not be EJSON compatible. In some odd cases, it might be necessary to change all 'undefined' values to something else, or resort to alternative representations like what `stringifyDDP` does. This patch makes `undefined` EJSON compatible.
I didn't remove the extra massaging in `stringifyDDP` and `parseDDP`, not knowing how set in stone the specs for DDP are, but it would simplify the codebase to do so.

#### 5) `clone` redundancy
Implementing `clone` feels to me to be completely redundant with implementing of `{to|from}{E}JSONValue`. This patch makes EJSON.clone work for any EJSON compatible type, even those that won't define a `clone` function. Note that it could be made more efficient if compatibility with `toJSONValue`/JSON factory was removed.

#### 6) `equals` redundancy
The same thing can be said about implementing `equals()`. Indeed, if the result `toEJSONValue` of two objects is the same, then they should be considered equal (since the client and the server will consider them equal), while if they are different, then we can only assume that the objects themselves are different. This patch makes `EJSON.equals` work for any EJSON compatible type, even those that won't define an `equals` function. Again, this could be made more efficient if compatibility with `toJSONValue`/JSON factory was removed.

#### 7) EJSON.equals symmetry
For some values `x` and `y`, it is possible that `EJSON.equals(x,y) !== EJSON.equals(y,x)`. A simple patch fixes this bug.

#### 8) Automatic helper
I would imagine that the implementation of `{to|from}EJSONValue` will be very similar for most custom types and depend solely on which attributes to serialize. It's pretty easy to add a helper to automatically create these functions. Here's a proposal for `EJSON.defineType`.

It reduces the 36 lines of [this example](https://github.com/EventedMind/meteor-create-a-custom-ejson-type/blob/master/app.js#L26-62) to a single line:

EJSON.registerType(Address, 'Address', ['city', 'state'])

It will also handle automatically attributes that are themselves of custom types, so this simplifies [this answer](http://stackoverflow.com/questions/17598811/can-i-embed-an-ejson-object-inside-another-ejson-object) to a single paragraph...

Before updating the documentation, etc., I thought I should put this PR out for comments and ideally insight from the core team as to the exact direction to aim for.

Thanks