jbmusso / gremlin-javascript

JavaScript tools for graph processing in Node.js and the browser inspired by the Apache TinkerPop API
MIT License
214 stars 63 forks source link

[WIP] Handle all things asynchronous internally with RxJS #50

Open jbmusso opened 8 years ago

jbmusso commented 8 years ago

I'm trying to lower the size of the dependency as well as make it easier to add new features (throttling, auto reconnect, etc.). My goal is:

All incoming and outgoing messages are now handled with RxJS streams. I'm unsure about the performance but I don't think this should be a concern when compared to network I/O.

Observable are essentially higher level streams that can be converted to Node.js Stream. They are currently a stage-1 candidate for addition to the EcmaScript standard. Upcoming RxJS v5 Observable tries to be compliant with the EcmaScript Observable Spec. I added rx v4 dependency since it's stable and offers more higher level methods absent in rxjs v5, especially .pausableBuffer() (absent in v5) which is quite handy for buffering outgoing messages until the WebSocket connection is actually open. I should add that regarding package names, rx is stable v4 on npm while rxjs is experimental/beta v5 (or so I figured ?!).

This cleans up the code quite a bit and also removes the executeHandler thing which I've never been a fan of.

Most tests pass, except those related to the previous Stream API obviously (.stream() and .messageStream()). I currently added a public (though undocumented yet) .observable() method which currently returns an RxJS stream. .execute() now calls .observable() internally.

Question : do people even use .stream() and .messageStream()? Even though these are public and documented methods, they've also been used internally by .execute() (which I believe most people use). By using RxJS, these two stream methods will no longer be necessary internally and I could just drop them from the public API altogether. It's also worth noting that Observable can be easily converted to Node.js Streams with a third party library https://github.com/Reactive-Extensions/rx-node (I don't think it's worth adding a dependency for this considering how trivial it is).

This PR is heavily experimental and I'll most likely send another clean PR when confident. This will most likely be a major version bump (3.x) depending on whether we want to keep the old Stream API or not.

Thoughts welcomed. Yeah, I'm looking at you @PommeVerte ;).

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-12.3%) to 79.479% when pulling 1e1c2431fc4552bf1f430f67acd39c92c3cd4647 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-12.3%) to 79.479% when pulling 4926513564b58e9297c28e5b4ee14b578477c428 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-11.8%) to 80.0% when pulling 4926513564b58e9297c28e5b4ee14b578477c428 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-11.8%) to 80.0% when pulling 4926513564b58e9297c28e5b4ee14b578477c428 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-6.7%) to 85.017% when pulling cdefe090bbba3ed5d8712a6335025b70a158a2cb on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-6.8%) to 84.965% when pulling ad0fb9446d1b12301d99a312aace0a3a29c60ee5 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-6.8%) to 84.965% when pulling 78490a5b59ac0ac5735ebb090702a6ab90ee16e8 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-6.8%) to 84.965% when pulling a0b1ba894604adcb9b29c79a58ef9a075077e2aa on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.

dmill-bz commented 8 years ago

Hmm I use executeHandler to parse data from specific server side serializers. https://github.com/PommeVerte/gc-graphson-text-plugin/blob/master/src/GraphSONTextPlugin.js#L19-L54

Looking at the code I guess this would be equivalent to overriding observable (although there is no easy way of doing that at the moment without extending). Also, this makes a line of assumptions on what the server will return (in handling statuses etc.). Of course that's also the case of the current code. I think the best way to be certain that all cases are handled would be to separate de (de)serialization process into it's own class and provide the client with a serializer loader. This way the client handles a generic Message class (or Request + Response) and users can simply load their custom serializers.

Thoughts?

jbmusso commented 8 years ago

I like the idea of separating the serialization/deserialization process. Let me give it more thought as I'm still getting started with Observables.

Also, the bundle size is huge with RxJS v4 as there is no way to cherry-pick methods. I'll have to switch to RxJS v5 and reimplement pausableBuffer().

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-6.4%) to 85.374% when pulling 6aec3f78f2db0352147f4eb5c315c1473d3d9bda on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.

jbmusso commented 7 years ago

I'm still eyeing toward this by the way (cc @guyellis @princjef). I think it'd be a very nice way to implement a database driver and it will allow the addition of tons of new higher level features very easily. I was thinking about:

Observable are just so powerful.

guyellis commented 7 years ago

Question : do people even use .stream() and .messageStream()?

I only use execute

jbmusso commented 7 years ago

I'm also working on https://github.com/jbmusso/zer which will basically allow us to do a RemoteGraph thing in JavaScript. The lib uses ES2015 Proxy object and can generate arbitrary strings of Gremlin with bound parameters. There's also partial support for DSLs right in JavaScript :).

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 1cc986a2f029bd4f95cfeabbdbba3884caece7a8 on rxjs into on master.