Using EJSON .addType() with DDP client?

[vsivsi]

Hi, I'm trying to use DDP to communicate with Meteor using method calls. One of my methods returns a Meteor ObjectID (EJSON type oid). Everything works okay if I turn EJSON support off, except that dates and oids are not sent/recieved in the correct formats. Turning EJSON support on fixes the date type, but introduces a crash:

/Users/vsi/jobWorker/node_modules/ddp/node_modules/meteor-ejson/ejson.js:215
      return converter(obj.$value);
             ^
TypeError: undefined is not a function
  at Object.builtinConverters.fromJSONValue (/Users/vsi/jobWorker/node_modules/ddp/node_modules/meteor-ejson/ejson.js:215:14)
  at fromJSONValueHelper (/Users/vsi/jobWorker/node_modules/ddp/node_modules/meteor-ejson/ejson.js:313:28)
  at /Users/vsi/jobWorker/node_modules/ddp/node_modules/meteor-ejson/ejson.js:286:21
  at Function._.each._.forEach (/Users/vsi/jobWorker/node_modules/ddp/node_modules/meteor-ejson/node_modules/underscore/underscore.js:86:24)
  at EJSON._adjustTypesFromJSONValue (/Users/vsi/jobWorker/node_modules/ddp/node_modules/meteor-ejson/ejson.js:284:5)
  at Object.EJSON.fromJSONValue (/Users/vsi/jobWorker/node_modules/ddp/node_modules/meteor-ejson/ejson.js:325:5)
  at Object.EJSON.parse (/Users/vsi/jobWorker/node_modules/ddp/node_modules/meteor-ejson/ejson.js:337:16)
  at [object Object].DDPClient._message (/Users/vsi/jobWorker/node_modules/ddp/lib/ddp-client.js:117:18)
  at [object Object].<anonymous> (/Users/vsi/jobWorker/node_modules/ddp/lib

I've figured out that this is because the EJSON package doesn't have a built-in custom type handler for oid. I have found the Meteor code implementing this (https://github.com/meteor/meteor/blob/devel/packages/minimongo/objectid.js) but don't see any way to get ahold of the EJSON object used by the DDP package to register this code using EJSON.addType() Is there some way to do this that I am missing, or do I need to change the DDP package to accomplish this?

[emgee3]

You're not missing anything, adding custom types wasn't really thought through. Would happily accept a pull request.

[vsivsi]

Do you have a preference of:

• Add a method to the DDP object: ddp.ejsonAddType() This is simpler
• Just expose the whole EJSON object: ddp.EJSON. This is more flexible

EIther way, this would only exist when ddp.use_ejson == true

Also, I'd love to just integrate support for the ObjectID type into the DDP package. SInce Meteor.Collection.ObjectID(...) is a fully supported EJSON type in Meteor, I feel like this will come up a lot, so maybe having it "just work" is a good thing.

The alternative would be for me to throw it into a separate npm package meteor-object-id and use it like:

ObjectID = require('meteor-object-id');
ddp.EJSON.addType('oid', function(str) { return new ObjectID(str); });
// or perhaps: ddp.ejsonAddType

Thoughts? Preferences?

[emgee3]

I agree that ObjectID should just work, considering for a period of time that was the default for _ids and it's likely that an app that's been around would have some in the database, even if they're not currently being created.

ObjectID support could be put into a separate repo, and I can create a new repo in oortcloud if you want to do this way. I'm not sure which way would be better. It seems pretty useless by itself, but then again, it is an add-on to the EJSON package. I was already planning to create a ddp-ejson repo and npm module to replace the existing meteor-ejson package because the existing one we use is unmaintained and out of date.

Whether or not EJSON is exposed or there's a method for adding custom types, I have no preference. Whichever works for you.

[vsivsi]

In Meteor 0.8.1.x the option to have collections automatically use ObjectIDs was added: http://docs.meteor.com/#meteor_collection (note the idGeneration: "MONGO" option). The default remains 'STRING' but ObjectIDs are definitely officially supported now and going forward.

Full EJSON support for ObjectIDs is 75 lines of code (including the ability to create new random ones.) Hardly seems worth putting in its own npm package... The new ddp-ejson package seems like a good home. Do you have a timeframe for doing that?

[emgee3]

Just published the updated fork. https://github.com/oortcloud/ddp-ejson

[vsivsi]

Great, I just created a PR over there for ObjectID support. I'll create another PR for this repo later tonight.

[vsivsi]

Okay, I just committed the EJSON changes here. Thanks for push access.

[emgee3]

Awesome! You probably noticed I got around to implementing DDP pre2 as well.

I'm a bit worried that all the changes will break Meteorite, which is probably the biggest user of the DDP client. I couldn't run the Meteorite tests with the new ddp-client (or the old for that matter), so I pushed a change to Meteorite's package.json to prevent it from picking up the 0.5.0 branch of ddp client.

Hopefully that gets published soon. I'll publish 0.5.0 of ddp once that occurs. Let me know if this causes you any problems.

[vsivsi]

That's fine. I'm working off a tag in my fork for now.

Breaking mrt would be a Very Bad Thing, so an abundance of caution is warranted. I've been using the master with a bunch of my own tests and it seems fine, but mrt obviously has a lot more cases to consider.

[emgee3]

Ok, mrt is updated and I pushed 0.5.0 of ddp.

[vsivsi]

Thanks!