Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

jbmusso
Copy link
Owner

@jbmusso jbmusso commented Apr 16, 2016

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 jbmusso force-pushed the rxjs branch 3 times, most recently from 1e1c243 to 4926513 Compare April 16, 2016 09:11
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.3%) to 79.479% when pulling 4926513 on rxjs into 5cc8c1c on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-11.8%) to 80.0% when pulling 4926513 on rxjs into 5cc8c1c on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-11.8%) to 80.0% when pulling 4926513 on rxjs into 5cc8c1c on master.

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@dmill-bz
Copy link
Collaborator

dmill-bz commented Apr 16, 2016

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
Copy link
Owner Author

jbmusso commented Apr 16, 2016

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
Copy link

Coverage Status

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

@jbmusso
Copy link
Owner Author

jbmusso commented Mar 31, 2017

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.

@guyellis
Copy link
Contributor

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

I only use execute

@jbmusso
Copy link
Owner Author

jbmusso commented Mar 31, 2017

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
Copy link

coveralls commented Jun 4, 2017

Coverage Status

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants