Introduce 'disableLosslessIntegers' config option

[lutovich]

This boolean option allows users to make driver accept and represent integer values as native JavaScript Numbers instead of driver's special Integers. When this option is set to true driver will fail to encode Integer values passed as query parameters. All returned Record, Node, Relationship, etc. will have their integer fields as native Numbers. Object returned by Record#toObject() will also contain native numbers.

Warning: enabling this setting can potentially make driver return lossy integer values. This would be the case when database contain integer values which do not fit in [Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER] range. Such integers will then be represented as Number.NEGATIVE_INFINITY or Number.POSITIVE_INFINITY, which is lossy and different from what is actually stored in the database. That is why this option is set to false by default. Driver's Integer class is able to represent integer values beyond [Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER] and thus is a much safer option in the general case.

Fixes #322
Related to #245, #225, #122

[lutovich]

I'm unsure about two things:

1. Is useNativeNumbers a good name for this setting? @nigelsmall could you please assess this?

2. What to do with TypeScript declaration files? This setting basically changes what kind of type driver uses for integer values. So TS type info depends on this setting. Things like:

   declare class Node {
     identity: Integer;
     ...
   }

   will have to change somehow to represent this. Maybe it's enough to use union types like this:

   declare class Node {
     identity: Integer | number;
     ...
   }

   and add comments? @pocesar, @TimothyDespair, @cojack could you please suggest?

[ali-ince]

As per your points;

1. I would prefer forceNativeNumbers because of its emphasise on forcing something that could go wrong (be lossy).

2. AFAIK, we can either specify any or union types as you've already suggested. Going with union types feel the way to go.

[ali-ince]

Changes look good to me.

What would you think about adding an toObject() overload which will convert Integer instances to number types although Driver is not configured to use native numbers?

[oskarhane]

This looks good to me as well.

And I agree with the changes that if you opt out of using the integer type when receiving results you should also not send integer types as params.

[oskarhane]

btw, I'm +1 on @ali-ince param name suggestion forceNativeNumbers

[sarincasm]

agree with what @ali-ince has suggested - 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})

also, the param could specify that we are forcing JS Numbers. Native might be confusing as in - native to javascript or native to neo4j - hence forceJSNumbers

[pocesar]

@lutovich you could make it a generic, so the person decides what type it should use, as such:

declare class Node<T = Integer> {
  identity: T;
  // ...
}

that means, if the person decides to not explicitly decide which type Node should be, it will be Integer (the default argument). If the person wants something else, he can then const node: Node<number> or type MyNode = Node<number>, and the types will be infered

[lutovich]

@pocesar thanks a lot for your reply! The only problem I see with Node<T = Integer> is that users will be able to use Node<string> and try to access identity as string, while it's in fact Integer | number.

Here is a possible solution:

declare class Node<T extends (Integer | number) = Integer> {
  identity: T;
  // ...
}

it's essentially the same thing but adds an additional generic type bound. So no code change for existing users, since Integer is still the default generic type. Users who enable JS numbers will have to use Node<number>.

What do you think?

[lutovich]

PR updated and should now be ready for a final review. Changes:

• renamed useNativeNumbers to disableLosslessIntegers
• lifted restriction on storing Integer when disableLosslessIntegers=true because it allows for better backwards compatibility
• updated TS declarations to support number instead of Integer

See commit messages for more details.

@eelsweb we've decided to only expose disableLosslessIntegers config on the driver level for now. Having it on both driver and record level makes things more complicated. What if it's set to true on the driver level but user calls record.toObject({disableLosslessIntegers: false})? It's definitely implementable but weird.

This change will hopefully be merged in 1.6 and go through alpha, beta and RC stages. Feedback is very valuable!

[pocesar]

@lutovich if the identity member will ALWAYS be either Integer or number, then it's the best choice to do that 👍
just a nit-pick, you can also explicitly decouple that from the declaration by creating a new type, like

declare type NumberInteger = Integer | number;
declare class Node<T extends NumberInteger = Integer> {
}

so it can be reused elsewhere

[sarincasm]

@lutovich this kind of over-riding is actually quite standard practice

For instance - Google's Nodejs Lib for their APIs

View the section Options

You may specify additional options either in the global google object or on a service client basis.

You can specify an auth object to be used per request. Each request also inherits the options specified at the service level and global level.

For this neo4j library, consider a case where 80 pc of my functions work with disableLosslessIntegers=true, but there are some cases where i need Integer, so I create a driver with {disableLosslessIntegers:true} and for the remaining few cases, I don't need a separate driver, I can simply override that by calling toObject({disableLosslessIntegers: false}) since local config takes precedence over global options.

[lutovich]

@eelsweb is this an actual use-case you have or just a theoretical concern?

Imho disableLosslessIntegers is a good first step. It solves the problem many people complained about. Overrides on the Record level are definitely doable but will make API more complicated. It is now unclear if they are needed or solve real problems. That is why I think we can defer them for future, at least right now.

[sarincasm]

Agreed 😄