Skip to content

Make the library more javascript friendly #322

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

Closed
sarincasm opened this issue Jan 15, 2018 · 5 comments
Closed

Make the library more javascript friendly #322

sarincasm opened this issue Jan 15, 2018 · 5 comments

Comments

@sarincasm
Copy link

One of the principles of a js client library should be to make it easier to use the underlying platfrom (in this case cypher/neo4j) from a js/nodejs application. I think there are a number of design patterns in this lib which make it quite impractical

This forces me to map the result in 100 pc of the cases - which IMO is one of the jobs that a client library should handle.

I don't mean to suggest that the other cases should be ignored, but these cases which are used less frequently and only in advanced/edge scenarios, should not be the default behavior.

@lutovich
Copy link
Contributor

Hi @eelsweb,

Thanks for raising this issue. I can't see how it is immediately actionable for us because, as you say, those problems have been mentioned before and corresponding issues are still open. There are number of problems with your suggestions:

  • both changes are backwards incompatible and can only be implemented in a major release
  • integer handling with special Integer class covers all use-cases, not just most. Native numbers will be a problem for applications that use JS to present data but other languages with wider integer types to insert data in the database. This is a general purpose library and it can't throw or return NaN for large numbers by default
  • returning JS objects instead of Records will require users to always access elements by name. Which means all Cypher queries will have to contain aliases, otherwise keys can get messy like result['count(n)']. Records however allow you to access elements by index so aliases in queries are optional

Could you please share your code for result mapping? It would be valuable for us to see how much of a burden is it.

@sarincasm
Copy link
Author

Thanks for your response.

  • On Integer handling - I agree it handles all cases. But my point is - what should be the default behavior ?
    I remember glancing through the readme section - note on numbers and the Integer type, and thinking - okay I need to be mindful in cases where we have very large numbers. I was then quite surprised to see a number like 17 represented as high and low bits.
    Should we expect 100 pc of applications using this library to handle this case, or should that burden fall on advanced users.

  • On response format -
    I am personally a fan, in general, of the format that gives me a keys array as it makes it quite easy to iterate through an object I am receiving from an api, regardless of what keys the object may have. However, this is more useful if I am unaware of what keys I am expecting from an api/query. In the case of a cypher query, if I get a count(n) that is because I wrote a query and would expect to receive a count(n) in my application, and should be expected to handle it as such.

Records however allow you to access elements by index

I am not sure If I understood the usage you are suggesting here - but it would be something like if my query has count(n) as the first element in my return statement,
then I can access the key count(n) by using - record.keys[0]
IMO this can actually get quite confusing as my indices always need to be correctly mapped to the order of the query - again not a very JS way of doing things.

  • Backwards compatibility
    I agree. I primarily opened this issue to try and influence development of this library in the future, especially since it is the official driver from neo4j and the thingdom library has now ceased development in favor of this one.
    However, the features can still be implemented in a backward-compatible way by using configurable options to the constructor (and clearly adding these to the quickstart boilerplate examples).

Our mapping function utilizes the toObject function and the transformation logic you had shared in this issue #225 - quite straightforward, but I need to use it after every query - something that IMO a client library should be able to handle internally (atleast with some one-time configuration).

Again, thanks for clearly responding to the concerns. As a user of this lib, I just wanted to point out some of my suggestions

@lutovich
Copy link
Contributor

Hi @eelsweb,

I agree that it's possible to introduce a config like useNativeNumbers on the driver level and use JS Numbers instead of Integer class. Overflows will then result in Number.NEGATIVE_INFINITY or Number.POSITIVE_INFINITY being returned.

Record class has a toObject() function, which right now returns Integers as-is. However with useNativeNumbers it will return a native JSON-like objects which only contains primitive JS and graph types ( like node, relationship and path). So there will be no need to apply that transformation logic. What do you think about this?

Records however allow you to access elements by index

I only mean that it's now possible to have query like ... RETURN a, b, c and access c in a record like this record.get(2) where 2 is a zero-based index of a record element.

@lutovich
Copy link
Contributor

lutovich commented Jan 17, 2018

@eelsweb attached PR adds a setting to make driver always use native JS numbers instead of Integer to represent integer values. With this setting set to true, function Record#toObject() will return a regular JS object with integer values represented as native numbers. So there will be no need to apply that transformation function and conversion form Record to object will be super simple.

Am I right that this resolves both your concerns?
Your PR review would really be appreciated!

@sarincasm
Copy link
Author

sarincasm commented Jan 20, 2018

@lutovich this is great. I think this should cover all cases while providing a lot of flexibility. Though I would also recommend adding this, as well as the use of toObject prominently to the README.

I think I also agree with what @ali-ince has suggested in the PR #323 - this config option could also be received by Record.toObject() (and maybe also by Record.get())

Lets say majority of my cases can be handled by JS Numbers and in a few cases I need the Integer -

constructor -
neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"), {forceJSNumbers: true})
handle edge case with the same driver
record.toObject({forceJSNumbers: false})

If, its the other way around,

constructor -
neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"))
handle smaller cases with the same driver
record.toObject({forceJSNumbers: true})

Thanks a lot for enhancing this so quickly. Waiting for 1.6 to drop now 😬

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

No branches or pull requests

2 participants