Use native BigInt type in Node 10+

[gabegorelick]

Now that Javascript has a native BigInt type, it may be time to rethink the decision to return large integers as strings.

node-pg could even fallback to a polyfill like https://www.npmjs.com/package/big-integer for older versions of Node.

[bendrucker]

Neat, didn't know about this!

big-integer and others don't seem like true polyfills to me. Even if they use BigInt internally if available, they don't return values that act like a BigInt.

So we're in a bit of an odd position where it would be nice for node 10 users to have BigInts but we can't make that backwards compatible for <10. If you upgrade your app from node 8 to 10, do the types suddenly change? I don't think that's ok. That would mean adding BigInt support as opt in, e.g. types.bigInt() auto-configures it. Happy to review a PR there.

[gabegorelick]

That would mean adding BigInt support as opt in, e.g. types.bigInt() auto-configures it.

I agree that this is the best path forward. That also makes the transition less painful if we eventually cut a new major version that dropped support for Node < 10, but that's probably a few years out.

big-integer and others don't seem like true polyfills to me. Even if they use BigInt internally if available, they don't return values that act like a BigInt.

True, but if the alternative is to use strings on Node < 10, then that's even less like BigInt. But I suppose we can leave it up to the client to decide if they want to opt in to a polyfill.

[Bessonov]

Even after reschedule EOL for Node 8, it ends this year. So, a major release with native BigInt support is reasonable for me. Also AWS Lambda supports it. That's a sign that Node 8 is really old :D

@bendrucker @brianc What do you think about drawbacks with:

import { types } from 'pg'
types.setTypeParser(20, BigInt)

?

[bendrucker]

pg currently supports node 4. I think it’s reasonable to track LTS but I’d like to do that by project policy rather than evaluating on a per-release basis. Please open an issue about that on the main pg repo and we can discuss and document accordingly.

[brianc]

I try to keep pg supporting any LTS release which is still in its support window. Looks like I could drop support for node 4 and node 6 as they've both left the LTS window.

[vitaly-t]

B.T.W. I'm dropping pre-ES2017 support right now.

[vitaly-t]

I have done some research in this area, as I am currently adding support of BigInt within pg-promise, see this issue.

First of all, since BigInt support was officially added in Node.js v10.4.0, it is simply too new to be breaking compatibility with so many clients out there. I'd say, we should give it another couple years at least, before considering such a change. I'm currently supporting Node.js v7.6.0 as the minimum, because it was the first version that made support for async/await official, which is indeed a huge milestone, and a very important aspect when writing database client code.

And since this driver does not do its own query formatting, its support for BigInt is already there.

For regular BigInt support, you just do this:

pg.types.setTypeParser(20, BigInt); // Type Id 20 = BIGINT | BIGSERIAL

And also to get arrays of BigInt natively, you add this:

// 1016 = Type Id for arrays of BigInt values
const parseBigIntArray = pg.types.getTypeParser(1016);
pg.types.setTypeParser(1016, a => parseBigIntArray(a).map(BigInt));

And that is it, the rest should just work as it is.

[gabegorelick]

Node 8 reaches end of life on December 31, 2019. At that point all active Nodejs releases will have BigInt.

[vitaly-t]

At that point all active Nodejs releases will have BigInt.

Node.js v10.4.0 onward, actually.

Also, as per comments above, enabling BitInt support in the current driver is pretty much a no-effort. With that in view, I believe nothing needs to be done here.

[gabegorelick]

Node.js v10.4.0 onward, actually.

Yes, but I believe v10 went LTS at 10.13. I would be surprised if there were still an appreciable number of clients running an older version of v10 before the LTS. If there are, I sure hope they're getting patched!

Also, as per comments above, enabling BitInt support in the current driver is pretty much a no-effort. With that in view, I believe nothing needs to be done here.

It's fine if this issue is closed as "won't fix". But I think there is an opportunity for node-pg to change its defaults. If it were written from scratch today, I see no reason why it wouldn't default to using BigInt (ignoring the LTS debate for a moment).

[vitaly-t]

I would be surprised if there were still an appreciable number of clients running an older version of v10 before the LTS. If there are, I sure hope they're getting patched!

Lots of them are old, and won't be patched.

But I think there is an opportunity for node-pg to change its defaults...

Such "opportunity" would require major version change, as it would break huge number of clients out there, while at the same time bringing in nothing new, because BigInt can be used already as it is.

[gabegorelick]

while at the same time bringing in nothing new, because BigInt can be used already as it is.

The "something new" is that node-pg does what you expect out of the box.

[vitaly-t]

The "something new" is that node-pg does what you expect out of the box.

You would only expect it, if you do not know how 64-bit numbers are handled in JavaScript, and nuances of switching between 53-bit number and 64-bit BigInt. And since number and BitInt are not even directly compatible with each other (needs explicit conversion), it is fair to say, that expectation is wrong, not this library.

For example, you can multiply INT by BIGINT in PostgreSQL directly. You cannot do the same in JavaScript, you need to convert the type first. That brings in a lot of implications, and extra complications.

[brianc]

Yeah I agree w/ @vitaly-t ... it's a huge change in a lot of subtle ways. What happens to all your queries which were doing JSON.stringify(results) and passing them to the browser? What about browsers that don't support it? reference

There are other edge cases too like BigInt behaving differently as keys in indexeddb, math between bigint and non-bigint numbers. I don't think we'll do this conversion by default for long time, but as @vitaly-t pointed out it's very easy to enable it. I also like @bendrucker's idea of making it an even easier "on switch" in pg types...but doing it by default introduces so many edge cases and footguns I don't imagine it will be a thing for at least a year or two.

[charmander]

I think the great thing about it is that the changes aren’t subtle. JSON.stringify will throw a very specific error, doing dangerous mixed math will throw a very specific error, and so on. Whereas right now…

• anyone doing math on count(*) with the defaults will get the right result most of the time, but could accidentally concatenate or produce a different type after only minor changes

• overflow is always a problem, and is equally silent

BigInt adds a layer of type safety, can be converted to a string or number for perfect compatibility with any existing operation, and is just as easy to opt out of through type overrides.

[gabegorelick]

What about browsers that don't support it?

Not sure I follow. As @charmander mentioned, you need to decide how you want to serialize BigInts anyway. If using JSON, that means they'll still typically be represented as strings.

Or does node-pg need to support running in browsers? I assumed from the name that it only ran in Node :)

[brianc]

yeah it only runs in node (as far as I know! someone might have gone crazy w/ webpack hacks at some point?) but i just mean its common in apps to take query results and serialize them to JSON and there's weirdness w/ json and bigint. But yeah...I definitely have never nor do I plan on supporting running node-postgres in the browser - if that's something someone wants to do...more power to 'em. 😄

[charmander]

So although I don’t understand how IndexedDB could be a problem if BigInts from pg can’t make it to the browser without explicit choices, the same concept does apply to Maps. new Map([[1n, 2]]).has(1) === false with no error, and generally 1n !== 1 with no error.

Still, waiting doesn’t make it less painful in the future. Maybe the next major version can make it opt-in with a single function call, with a deprecation warning if you don’t opt in explicitly opt in or opt out?

[brianc]

Still, waiting doesn’t make it less painful in the future. Maybe the next major version can make it opt-in with a single function call, with a deprecation warning if you don’t opt in explicitly opt in or opt out?

I think that's a cool idea.

[pauldraper]

polyfill like https://www.npmjs.com/package/big-integer

FWIW I think https://github.com/GoogleChromeLabs/jsbi is the superior choice for BigInt.

types.bigInt() auto-configures it

I agree that the best thing to do is make native bigint usage customizable -- which it already is, but build-in the option to the library.

JSON.stringify

The error is a new, but default JSON.parse/JSON.stringify could never round-trip all node-pg types anyway (e.g. Date).

[vitaly-t]

@pauldraper Actually, in case of BigInt, round-tripping is easy, which is what I did within pg-promise. First, see my earlier post above, where I show how easy it is to activate BigInt conversion from server to the client. And then you can provide a custom JSON converter, like I do here. See also full feature page I wrote about it.

[bendrucker]

PR welcome here. Based on the factors discussed here and in brianc/node-postgres#2398 this is a reasonable default.

I'm removing this from the 4.0 milestone because I don't have time to work on it right now and don't want to continue holding up other breaking change PRs that were submitted and merged.