Add support for BigInt #696

bigmontz · 2021-03-29T15:03:27Z

Recent V8 release adds a new type - BigInt. It is thus available in latest NodeJS (v10+) and Chrome (v67+).

Driver could use this type instead of the custom Integer to represent lossy integer numbers. This is configurable because BigInt and Number are not fully compatible, so driver can't always return BitInts. For example BigInt(1) / Number(1) results in a TypeError, as well as JSON.stringify({v: BigInt(1)}).

The configuration property is useBigInt and it's false by default, in manner to break drivers user on update.

This configuration take precedence over disableLosslessIntegers

Compatibility matriz: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

fbiville · 2021-03-29T15:54:13Z

Are we OK with not supporting IE if big int usage is enabled?

bigmontz · 2021-03-29T15:59:37Z

Are we OK with not supporting IE if big int usage is enabled?

I will check what's happen when it is not available and the flag is disabled. It should not fail.

Recent V8 release adds a new type - BigInt. It is thus available in latest NodeJS (v10+) and Chrome (v67+). Driver could use this type instead of the custom Integer to represent lossy integer numbers. This is configurable because BigInt and Number are not fully compatible, so driver can't always return BitInts. For example `BigInt(1) / Number(1)` results in a TypeError, as well as `JSON.stringify({v: BigInt(1)})`. The configuration property is `useBigInt` and it's `false` by default, in manner to break drivers user on update. This configuration take precedence over `disableLosslessIntegers`

bigmontz · 2021-03-30T16:53:05Z

@fbiville, I've check in environment which doesn't support bigint, and if the useBigInt is not enabled, the BigInt constructor is not called and nothing undesired happen. If you enable, it will explode.

oskarhane

Just one comment where I think we're missing a useBigInt check.

In general, I like the move towards this over the customer integer type we currently have.

Just a side note:
In most of the applications we build we use web workers to get the result and transfer the data via postMessage with the result serialized as JSON.

Surprisingly BigInt's can be serialized at the moment so this kinds of usages of the driver need to add a custom .toJSON() to the big int results. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#use_within_json

testkit-backend/src/cypher-native-binders.js

OskarDamkjaer · 2021-03-31T15:42:18Z

test/driver.test.js

@@ -26,6 +26,7 @@ import {
 } from '../bolt-connection/lib/pool/pool-config'
 import { ServerVersion, VERSION_4_0_0 } from '../src/internal/server-version'
 import testUtils from './internal/test-utils'
+import { json as JSON } from 'neo4j-driver-core'


It's a detail, any specific reason some imports shadow JSON here and change all uses to json instead?

Sorry, It's a leftover.

OskarDamkjaer · 2021-03-31T15:51:05Z

core/src/json.ts

+ * @private
+ * @param val The value to be serialized as JSON
+ * @returns the json
+ */


Perhaps this doc comment could be updated to something like:

Custom version on JSON.stringify that can handle values that normally don't support serialization, such as BigInt.

@param val — A JavaScript value, usually an object or array, to be converted.
@returns A JSON string representing the given value.

OskarDamkjaer · 2021-03-31T15:52:36Z

core/src/json.ts

+ */
+export function stringify (val: any) {
+  return JSON.stringify(val, (_, value) =>
+    typeof value !== 'bigint' ? value : `${value}n`


Personally I find it clearer to read ternaries as typeof value === 'bigint' ? `${value}n` : value

It makes sense, It was my C/C++ accent.

OskarDamkjaer · 2021-03-31T15:54:11Z

core/src/internal/temporal-util.ts

@@ -575,3 +580,12 @@ function formatNumber (
  }
  return isNegative ? '-' + numString : numString
 }
+
+function add (x: NumberOrInteger, y: number): NumberOrInteger {
+  if (x instanceof Integer) {


any reason functions have a space before listing arguments in this file?

It is in the repository codestyle and it's done automatically. But it's a bit useful when you're searching for methods use or declarations.

OskarDamkjaer · 2021-03-31T15:57:46Z

bolt-connection/test/bolt/bolt-protocol-v4x3.test.js

 import { internal } from 'neo4j-driver-core'

+const WRITE = 'WRITE'


Don't see why this variable is needed

OskarDamkjaer · 2021-03-31T15:58:31Z

bolt-connection/test/bolt/bolt-protocol-v4x0.test.js


 import { internal } from 'neo4j-driver-core'

+const WRITE = 'WRITE'


Not sure why this variable is needed, the string seems to be inlined in other changes

Actually I prefer this const solution, it's cleaner and easy to change if the value change.

OskarDamkjaer · 2021-03-31T16:00:46Z

I don't have a full view of how the driver/BigInt works, but looking through the code I didn't find anything that seemed off. I left some small code comments, feel free to disregard if you don't find them useful

bigmontz force-pushed the 4.3-support-bigint branch 2 times, most recently from 3b5787c to c9a1bbc Compare March 29, 2021 15:11

bigmontz marked this pull request as ready for review March 29, 2021 15:11

bigmontz force-pushed the 4.3-support-bigint branch from c9a1bbc to 956501e Compare March 29, 2021 15:30

bigmontz force-pushed the 4.3-support-bigint branch from 956501e to b1a11dc Compare March 29, 2021 15:55

bigmontz force-pushed the 4.3-support-bigint branch 2 times, most recently from 6af3d93 to 863c946 Compare March 29, 2021 16:45

use bigint to the core types

cfbbcc8

bigmontz force-pushed the 4.3-support-bigint branch from b0e42d8 to 9682974 Compare March 30, 2021 15:56

Fix bigint json serialization and remove wrong null/undefined checks

e61eef0

bigmontz force-pushed the 4.3-support-bigint branch from 9682974 to e61eef0 Compare March 30, 2021 16:49

bigmontz requested review from OskarDamkjaer and oskarhane March 30, 2021 16:53

oskarhane reviewed Mar 31, 2021

View reviewed changes

testkit-backend/src/cypher-native-binders.js Show resolved Hide resolved

improve useBigInt docs

8f30dcc

OskarDamkjaer reviewed Mar 31, 2021

View reviewed changes

Addressing PR comments

24c11af

bigmontz merged commit 2c1da8f into neo4j:4.3 Apr 1, 2021

bigmontz deleted the 4.3-support-bigint branch April 1, 2021 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for BigInt #696

Add support for BigInt #696

bigmontz commented Mar 29, 2021 •

edited

Loading

fbiville commented Mar 29, 2021

bigmontz commented Mar 29, 2021

bigmontz commented Mar 30, 2021

oskarhane left a comment •

edited

Loading

OskarDamkjaer Mar 31, 2021

bigmontz Apr 1, 2021

OskarDamkjaer Mar 31, 2021

OskarDamkjaer Mar 31, 2021 •

edited

Loading

bigmontz Apr 1, 2021

OskarDamkjaer Mar 31, 2021

bigmontz Apr 1, 2021

OskarDamkjaer Mar 31, 2021

OskarDamkjaer Mar 31, 2021

bigmontz Apr 1, 2021

OskarDamkjaer commented Mar 31, 2021

		import { internal } from 'neo4j-driver-core'

		const WRITE = 'WRITE'

Add support for BigInt #696

Add support for BigInt #696

Conversation

bigmontz commented Mar 29, 2021 • edited Loading

fbiville commented Mar 29, 2021

bigmontz commented Mar 29, 2021

bigmontz commented Mar 30, 2021

oskarhane left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarDamkjaer Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarDamkjaer commented Mar 31, 2021

bigmontz commented Mar 29, 2021 •

edited

Loading

oskarhane left a comment •

edited

Loading

OskarDamkjaer Mar 31, 2021 •

edited

Loading