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

v2 implementation! #145

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

v2 implementation! #145

wants to merge 121 commits into from

Conversation

aseemk
Copy link
Member

@aseemk aseemk commented Nov 7, 2014

Fixes #143.

API docs: https://github.com/thingdom/node-neo4j/blob/v2/API_v2.md

Core:
Cypher:
Schema management:
  • Labels
  • Properties
  • Relationship types
  • Indexes
  • Constraints
  • Legacy indexing (punted)
Advanced:
Testing:
  • Neo4j 2.1
  • Neo4j 2.2 (forced password reset, changed error formats)
  • Neo4j 2.3 (changed transaction error effects, removed index hint failure)
  • Node 0.10
  • Node 0.12
  • io.js (fixes Test on io.js #149)
  • Node v4 & v5
Documentation:

@brian-gates
Copy link

My two cents: examples should be standard javascript without streamline. I had to look up the documentation for what streamline does to understand your docs.

Also worth asking: what about streaming the response JSON? It looks like Oboe.js supports reading an existing HTTP stream (docs), but not in the browser. Is that fine?

Oboe.js does work in the browser :)

It seems to be implied that transaction requests immediately (i.e. synchronously) return a stream. This is not technically correct. Due to buffering and batching it is currently asynchronous.

tx.cypher {query, params, headers, raw, commit}, _

I also would prefer that headers, raw, and commit were bundled into an options param. That way you aren't forced to use null params if you want to commit without headers or raw.

@aseemk
Copy link
Member Author

aseemk commented Nov 7, 2014

My two cents: examples should be standard javascript without streamline.

Great point. Will fix.

Oboe.js does work in the browser :)

Does it have to be the one making the HTTP request though? The API docs seem to imply that the "bring your own stream" feature is only in Node — but do you know if that's actually true in practice?

http://oboejs.com/api#byo-stream

It seems to be implied that transaction requests immediately (i.e. synchronously) return a stream. This is not technically correct. Due to buffering and batching it is currently asynchronous.

Really? This example shows cypher.transaction() returning a stream right away:

https://github.com/brian-gates/cypher-stream#query-batching

I also would prefer that headers, raw, and commit were bundled into an options param. That way you aren't forced to use null params if you want to commit without headers or raw.

I'm not sure if I'm misunderstanding you, but typically, options in options objects can simply be omitted if they're not needed. So you'd never be forced to pass null params.

E.g. see jQuery.ajax(settings): http://api.jquery.com/jquery.ajax/

I'll clarify this in the doc though. And the current doc is just a rough outline of the API, but the final/proper API docs will definitely specify precisely which options are needed vs. which are optional. =)

@brian-gates
Copy link

Does it have to be the one making the HTTP request though? The API docs seem to imply that the "bring your own stream" feature is only in Node — but do you know if that's actually true in practice?

I've only ever tested it where Oboe is making the request. That works. Not sure about anything else.

Really? This example shows cypher.transaction() returning a stream right away:
https://github.com/brian-gates/cypher-stream#query-batching

The transaction stream is synchronous. Streams per statement are asynchronous, which is what your documentation appears to be referring to.

https://github.com/brian-gates/cypher-stream#stream-per-statement

I'm not sure if I'm misunderstanding you, but typically, options in options objects can simply be omitted if they're not needed. So you'd never be forced to pass null params.

Oh, ignore me, that's just CoffeeScript throwing me for a loop :)

@aseemk
Copy link
Member Author

aseemk commented Nov 7, 2014

Cool, thanks. I won't worry about Oboe in the browser then. Non-"mainline" scenario aside, there's also nothing fundamentally limiting; it should always be theoretically possible to still stream the JSON parsing even if you're not the one making the HTTP request.

Great point about statement streams. Any reason though that a stream couldn't be returned immediately, and "wired up" under the hood later whenever the underlying CypherStream is emitted?

(This assumes that we'd provide our own wrapper stream rather than the underlying CypherStream directly, but I think that's probably a wise API thing to do anyway. Or maybe we document that we're returning a CypherStream directly, and we just proxy all events, etc.)

Thanks for all the great feedback again!

@brian-gates
Copy link

Any reason though that a stream couldn't be returned immediately, and "wired up" under the hood later whenever the underlying CypherStream is emitted?

The streams API prevents me from returning anything synchronously from my write.

That said, I was planning on adding support for accepting a stream to pipe to as an alternative to callbacks. This would allow your library to generate and return a proxy stream immediately:

var stream = new require('stream').PassThrough(); // or you could use highland.
cypher({ statement: statement, pipe: stream });
return stream;

@aseemk
Copy link
Member Author

aseemk commented Nov 7, 2014

Nice. An API like that would be great. Thanks Brian.

@aseemk aseemk force-pushed the v2 branch 2 times, most recently from 5bd2994 to 3eeec0e Compare November 9, 2014 20:07
@aseemk aseemk force-pushed the v2 branch 8 times, most recently from 7e9efbc to 39f87de Compare January 30, 2015 07:36
@aseemk aseemk force-pushed the v2 branch 2 times, most recently from 8ba035a to f329d50 Compare April 5, 2016 13:33
@dozoisch
Copy link

dozoisch commented May 4, 2016

I'm all with @gasi ! We've been using this in prod for a while too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment