gremlin-javascript
gremlin-javascript copied to clipboard
[WIP] Handle all things asynchronous internally with RxJS
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:
- to remove
highland
andreadable-stream
dependencies - to add
rx
lightweight dependency since I'm only using very few Rx operators internally
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 ;).
Coverage decreased (-12.3%) to 79.479% when pulling 1e1c2431fc4552bf1f430f67acd39c92c3cd4647 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.
Coverage decreased (-12.3%) to 79.479% when pulling 4926513564b58e9297c28e5b4ee14b578477c428 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.
Coverage decreased (-11.8%) to 80.0% when pulling 4926513564b58e9297c28e5b4ee14b578477c428 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.
Coverage decreased (-11.8%) to 80.0% when pulling 4926513564b58e9297c28e5b4ee14b578477c428 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.
Coverage decreased (-6.7%) to 85.017% when pulling cdefe090bbba3ed5d8712a6335025b70a158a2cb on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.
Coverage decreased (-6.8%) to 84.965% when pulling ad0fb9446d1b12301d99a312aace0a3a29c60ee5 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.
Coverage decreased (-6.8%) to 84.965% when pulling 78490a5b59ac0ac5735ebb090702a6ab90ee16e8 on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.
Coverage decreased (-6.8%) to 84.965% when pulling a0b1ba894604adcb9b29c79a58ef9a075077e2aa on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.
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?
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()
.
Coverage decreased (-6.4%) to 85.374% when pulling 6aec3f78f2db0352147f4eb5c315c1473d3d9bda on rxjs into 5cc8c1c7ff83dc81b0abb1b35043531cfaa70bb6 on master.
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:
- multiple connections to multiple hosts/load balancing
- live metrics/monitoring (pending queries, outbound/inbound, etc.)
- automatic reconnection
- throttling
- ...
Observable are just so powerful.
Question : do people even use .stream() and .messageStream()?
I only use execute
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 :).