gremlin-javascript icon indicating copy to clipboard operation
gremlin-javascript copied to clipboard

[WIP] Handle all things asynchronous internally with RxJS

Open jbmusso opened this issue 8 years ago • 15 comments

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 and readable-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 ;).

jbmusso avatar Apr 16 '16 09:04 jbmusso

Coverage Status

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

coveralls avatar Apr 16 '16 09:04 coveralls

Coverage Status

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

coveralls avatar Apr 16 '16 09:04 coveralls

Coverage Status

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

coveralls avatar Apr 16 '16 09:04 coveralls

Coverage Status

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

coveralls avatar Apr 16 '16 09:04 coveralls

Coverage Status

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

coveralls avatar Apr 16 '16 09:04 coveralls

Coverage Status

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

coveralls avatar Apr 16 '16 10:04 coveralls

Coverage Status

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

coveralls avatar Apr 16 '16 10:04 coveralls

Coverage Status

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

coveralls avatar Apr 16 '16 10:04 coveralls

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?

dmill-bz avatar Apr 16 '16 12:04 dmill-bz

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().

jbmusso avatar Apr 16 '16 12:04 jbmusso

Coverage Status

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

coveralls avatar Apr 19 '16 21:04 coveralls

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.

jbmusso avatar Mar 31 '17 20:03 jbmusso

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

I only use execute

guyellis avatar Mar 31 '17 21:03 guyellis

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 :).

jbmusso avatar Mar 31 '17 21:03 jbmusso

Coverage Status

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

coveralls avatar Jun 04 '17 14:06 coveralls