Rewrite JSON API

[tedsuo]

• JSON Client passes tests, but surely has whatever previous bugs or quirks
• json.html in prototype server runs the code currently at https://github.com/share/ShareJS/wiki/JSON-Client-API

[josephg]

ot-types has been renamed to simply 'ottypes' - hence travis build failed.

[josephg]

Also, lets fix the old bugs while we're at it. The problem is that if you do doc.at(path1), then move path1 to path2, the subdoc object doesn't move.

[tedsuo]

ok, I'll add a test for that and resubmit.

[josephg]

@nornagon can you comment on this? Does this fix the problems you were having? I'm inclined to accept it.

[josephg]

Is this actually just the compiled output from the old code, or have you changed it? If the former, its still buggy. If the latter, please remove this comment. (In general, we should clean it up so it doesn't look like generated code)

[tedsuo]

Ok, I believe I fixed the list move issue.

• it's just list move, currently there's no object move, correct?
• I had to listen for 'op' events on context._doc in order to do it. Pretty sure that's "wrong" but not sure what the better option is.
• Listening for 'op' events means listener cleanup. I can add that if this is the right approach.

I also ported the sortable train list example to your prototype server. It appears to work, except the server does not seem to be saving the move operations for some reason.

[josephg]

• Yep, no object move for now.
• Why do you need to listen to context._doc.on('op')? The context has a _onOp: function(op){...} method that gets called on every operation already. You can also define a _beforeOp: function(){} if you need to look at the operation before its been applied to the document snapshot.

[tedsuo]

Right now _onOp is only triggered for remote operations. But I need capture the local operations as well. Perhaps that's a bug?

[josephg]

It should be triggered by any operation that comes from a different context. The operations that are applied locally by the JSON API itself we can probably capture directly...

[dgreisen]

We had discussed having ops fire regardless of whether they are local or
remote and providing a flag specifying local or remote.

This would greatly simplify many use cases. When I'm back at the computer
I'll find the thread where this was discussed.
On Jul 15, 2013 6:28 PM, "Joseph Gentle" notifications@github.com wrote:

It should be triggered by any operation that comes from a different
context. The operations that are applied locally by the JSON API itself we
can probably capture directly...

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/230#issuecomment-21015798
.

[josephg]

@dgreisen we implemented that for regular document op events. The _onOp handler for document contexts is a little different - if the doc context is:

{
  _onOp: function(op) { .... },
  dance: function() { this.submitOp('dance'); }
}

I don't think it makes sense for the local context functions (like dance) to in turn make onOp fire - and it seems lazy to need that. If you really want that behaviour, you could just do this anyway:

{
  _onOp: function(op) { .... },
  _submitAndEmit(op) { this.submitOp(op); this._onOp(op); }
  dance: function() { this._submitAndEmit('dance'); }
}

[tedsuo]

ok, this now works without accessing _doc.

btw, once all of this is considered acceptable, I can do a pass to de-cruft the coffeescript generated code.

[josephg]

Awesome.

Uhh - @nornagon ?

[tedsuo]

BTW, one thing this change introduces is the need to call destroy on your subdocs, or else you will leak as long as the context is around.

On the one hand that's fine, you need to call destroy on your context, and subdoc is basically a context, so it's not weird that you have to destroy it. But the current API makes it not obvious that you're creating state and context that's going to stick around, since it looks like you're just chaining method calls.

e.g. this looks like a fine way of using the api, but would cause leaks until context.destroy() is called:

context.at('monkey').set({ name:'bobo', banana_hoard:0 }); // can't destroy 'monkey' subdoc!!
my_monkey = context.at('monkey');
my_monkey.at('banana_hoard').add(1); // can't destroy 'banana_hoard' subdoc!!

This pull request is just about getting the json client working again on 0.7, not fiddling with the API, but I wanted to flag that.

[josephg]

Hm - interesting... I wonder what the best answer for this is.

[josephg]

... Obviously with weak maps in the next version of javascript we can get around that, but for now...?

[tedsuo]

Yeah not sure. I have ideas, but they break the current api and I'd like to actually use Share for a bit before I go around proposing things like that. Given that share is still alpha, maybe it's not a big deal for the moment?

One thing I noticed right out of the gate though, I would prefer subdocs to have all the getAt, insertAt methods that the context has. Then I can say:

my_monkey = context.at('monkey');
my_monkey.setAt( 'name', 'bonzo');
my_monkey.addAt( 'banana_hoard', 1);

that would get rid of the need for making a lot subdocs, and also make contexts and subdocs the same thing from the users point of view. It feels like subdocs are sort of an implementation detail; as a user I just want a context, and when I grab a node from lower down in the tree, that node is also a context.

[tedsuo]

Also, a small breaking change would be to rename 'at' to 'createAt'. Then a least it's obvious that it's a factory method, not an accessor.

[josephg]

Yep sounds good - and yeah, now's the time to make breaking changes :) (There's heaps of API changes with 0.7 anyway.)

[dgreisen]

I think it would make sense to accept this pull after the "coffee cruft" is cleaned up but before making changes to the api. Then we can start changing the api.

I like Ted's suggestion to rename at(). createAt() works, but perhaps getSubdoc() or getSubdocAt(), might be more descriptive since that's what you're doing? I also like your suggestion that we add a way to modify without creating new contexts. But I would suggest rather than add a whole duplicate set of api calls (set and setAt, get and getAt...), every api call can just have an optional path argument. Thus you would have subdoc.set(value, [path], callback). I think that is a pretty natural way of doing things and doesn't complicate the api too much.

There are a couple other changes I'd like to see in the api in order of priority:

1. path argument for every api call (see above)
2. optionally use jsonPath notation,or something similar, definitely for registering event handlers, and possibly for api paths.
3. an updateText() api method that would operate on a text node and would work just like textarea.applyChange() currently does in 0.6 branch.

I'd appreciate feedback on all three proposals. I'm happy to help with implementation if any of them are oked.

[tedsuo]

heyo david,

1. I'm fine with an optional path parameter instead of separate methods. Joseph, any preference?

for at(), how about createContextAt()? Then it matches doc.createContext(). I'd like it to be "create" instead of "get" so that it's clear you're getting a new one:

monkey1 = context.createContextAt('monkey');
monkey2 = context.createContextAt('monkey');
monkey1.destroy() // monkey2 will still work

2. jsonPath has wildcards, etc, which would dramatically complicate things under the hood for if we used it for paths. My vote would be to hold off on that until the api has been stable for a while. For events, I don't need it but it seems reasonable? The only thing I don't like is having multiple, subtly different event api's flying around in the same system, so I would want MicroEvents and everything else to have the same behavior.

3. I'm not sure what the state of affairs are regarding text in the json0 type, but yeah that seems like a good one.

[dgreisen]

at: is there a reason to create new contexts every time? conceptually to me when I call at('/path/to/a') twice, i'm trying to reach the same object at the same location, so they should return the same subdoc (not two subdocs pointing to the same thing). is there any reason we couldn't (or shouldn't) keep track of the contexts and return one context for each path? Not creating new duplicate objects always seems preferable. if this is not possible, i think something like createSubdocAt would be more explanatory to someone unfamiliar with the intricacies of javascript and sharejs.

jsonpath; I agree wrt jsonpath in paths. i also can't really think of a use case for it. But it would be nice to have optional dot notation for paths, and jsonpath would be incredibly useful for events. i semiregularly find myself wanting to create a handler for, say, every phone number in a contact. I end up having to create a child op listener and then filtering on paths and ops. but having it built in using jsonpath would be much cleaner.

updateText: there's nothing preventing this. i have already implemented it on top of 0.6 as a bootstrapped function. relevant line in code: https://github.com/share/ShareJS/blob/master/src/client/textarea.coffee#L7.

@josephg; any thoughts?

[dgreisen]

One more thing:

Events: The way events are handled by the subdoc is, I think, incredibly unintuitive, unless I am completely missing something. iirc, and it's been awhile since I looked into this, to describe a unique json event handler, you must provide an event, a method, and a path. When you call subdoc.on(event, callback) the subdoc registers a handler with these three properties on its parent document. It then returns a listener object. To remove a listener, you must keep track of the listener object and pass it to subdoc.removeListener(listenerObject).

There are many problems with this.

a) If i call subdoc.on to add a listener, I expect to be able to call subdoc.off to remove a listener. I cannot.
b) I also expect to be able to remove a listener without keeping track of a listener object. if i call subdoc.off(event, callback) that is all the information needed to remove the event, since the subdoc has the path.
c) since we are using microevent, we should use its conventions, which are bind and unbind.
d) Finally, I really like Backbone's ability to remove all events by calling off() with no arguments. or to remove all events handled by a certain method by calling off(null, handler), or to remove all listeners of a specific event by calling off(event). I would love to see this functionality throughout share.js and could simply be added to the microevent implementation.

[tedsuo]

So, the issue is that subdocs now have a destroy() method that must be called to prevent leaks. As soon as you have to destroy(), you're doing manual memory management, and you really want to be able to make independent copies so that different subsystems don't have to coordinate with each other; each can call destroy() when it's done doing it's thing without risk of messing the other guy up.

If there's a way to eliminate the need for destroy(), then at() makes sense, you are just accessing the part of the graph at that point. But if subdocs have the requirement that they can update themselves and move about in the graph, they have to observe the context above them for path changes. I don't know of a way to implement this observer pattern that doesn't require cleanup. So as long as we have destroy(), I think it should be createContextAt() or something similar.

Regarding events: yeah the issue you are having is share's events are modeled after node's event api, not backbone's. I agree that backbone has a much better system, especially for cleanup. With backbone you can say foo.off(null, null, this) which means "stop listening to whatever I was listening to on foo" which is really great.

[josephg]

I don't have a strong opinion about this API. I agree that if we're doing manual memory management we should call things create*() and destroy(). I'd like to see the generated coffeescript cleaned up, but I'm leaving it up to you @tedsuo to let me know when you think its ready.

I'm not familiar with backbone's events, though that off() thing looks pretty sweet. Do you have any docs to point me to? As you say, its pretty easy to add stuff to microevent if thats something we want to do.

[tedsuo]

Oh hey, I posted the cleanup already, forgot to mention it.

[dgreisen]

@josephg: backbone events docs are at http://backbonejs.org/#Events. But it is more useful to read the code: http://backbonejs.org/docs/backbone.html#section-13.

An even cooler aspect of backbone events, added in 1.0 is listenTo et al, which allow the listening object to also keep track of events, so regardless of context, all events dealing with that object can be guaranteed cleaned up on removal.

backbone events could easily replace microevents if 3 issues are addressed:

1. would have to add [underscore](http://underscorejs.org/] (5kb min/zipped) as a dependency. This is probably a good thing anyways, now that shareJS is pure javascript, as it can greatly simplify js coding. (could reimplement without underscore, though, if you don't want another dependency)
2. trigger and emit both have the same arguments, so would have to do a search/replace or alias emit to trigger. Same with off and removeListener. all existing calls to removeListener will work just as expected with off, since off just adds a third optional parameter.
3. big one for last: context. You are using context to pass information about the object emitting the event by always binding the handler's context to the object emitting the event. if you don't specify a context for a backbone event it defaults to a context of the emitting object (just like microevent). However, if you allow the on function to change the context, you need to pass the emitting object as an event attribute, regardless of the default context. I think this is a good idea anyways, as i find I have to use liberal use of backbone.proxy with sharejs events; after all, when an event triggers a handler on an object, I probably want to do something on that object, not the emitting object.

If this is something you would like to see happen, I'd be happy to work on it.

[dgreisen]

@tedsuo: I gave thought to subdocs all last night. I think you are correct, there is no way to create a proper subdoc without creating listeners on the doc for moves. So I vote for createSubdocAt().

[dgreisen]

I skimmed your pull, and i think it looks ready to merge once at is renamed.

Once this is merged, we can improve the api.

[tedsuo]

ok, so I made the following changes:

• context and subdoc have the same API
• at() changed to createContextAt()
• optional path and callback arguments everywhere
• insertAt(), getAt(), etc merged into insert(), get(), etc

At this point, subdoc is just wrapper methods on the context. They could be the same class, except contexts have to be mixed in to the type.api and so can't use prototypal inheritance.

I feel pretty good about it at this point, the API feels clean now. So I'll submit this as final unless there are objections.

[tedsuo]

Oh hey one thing Joseph. Uglify rather verbosely complains about how I'm doing polymorphism (checking the arguments array and adding defaults for missing path and callback params). There's no real problem as I can see but I couldn't find a way to tell it to shut up.

[josephg]

Blah sorry I totally dropped the ball on this. Nice work 💃

Don't worry about uglify - it whinges about heaps of my code too, and as you say there's no way to make it shut up.

[josephg]

All this _results, _results1 and the closure is totally unnecessary glup

[josephg]

... I cleaned this up a bit after pulling.

[dgreisen]

Thanks for the pull, @josephg, and thanks for all the work @tedsuo. I'm opening individual bugs for improvements that I'd like to see, and that I can work on. That way can keep the conversation more manageable. should we link all the individual issues from here to keep them in one place?

[josephg]

Go for it if you like - though a wiki page might work better if you feel you need it.

[tedsuo]

Thanks Joseph!

Wiki page sounds good to me! Here: https://github.com/share/ShareJS/wiki/0.7-JSON-Client-API