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

bigint: Just use a bigint type when applicable #90

Closed
wants to merge 2 commits into from
Closed

Conversation

mattrobenolt
Copy link
Member

This would be a large API breaking change, but I think it's the right thing to do. I'm not sure if this is safe to use, but from my little bit of research, BigInt is pretty ubiquitous across browsers and JS runtimes.

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

Refs #89

This would be a large API breaking change, but I think it's the right
thing to do. I'm not sure if this is safe to use, but from my little bit
of research, BigInt is pretty ubiquitous across browsers and JS
runtimes.

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

Refs #89
Comment on lines -329 to -333
if (value === '' || value == null) {
return value
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're being type aware, we can't just return an empty string or null for numbers, we can, but it's more correct to coerce them in their specific switch branches so they get a 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need this check for null values. The switch below acts on the type of the column but not the value returned for each row.

@iheanyi
Copy link
Member

iheanyi commented Mar 4, 2023

We thought about this when designing this. We use a string to let the callers handle it however they want. BigInt can't be strictly compared to regular numbers, so the caller is responsible for deciding how they want to handle / cast it.

@mattrobenolt
Copy link
Member Author

I get that. But I'd argue if someone wants to downcast it, they can the same way we suggest handling it as a bigint. It seems like a discredit to ignore that there's a primitive bigint available.

The issue imo stems from rowsAffected and lastId also being bigints, and we should treat them as such. If we're going to use bigint somewhere, it'd make sense to just do the right thing for each column type too.

Seems a bit bizarre to leave them as strings, since they also aren't really comparable without being coerced into something else either.

Base automatically changed from coerce-null to main March 6, 2023 17:19
@dgraham
Copy link
Member

dgraham commented Mar 6, 2023

I don't think we should return BigInt values by default because they're difficult to work with. For example, this would break any app that returns the data set as JSON.

JSON.stringify({a: 12})
// => '{"a":12}'

JSON.stringify({a: 12n})
Uncaught TypeError: Do not know how to serialize a BigInt
    at JSON.stringify (<anonymous>)

Once BigInt is introduced it must be used everywhere because operators are not supported.

12 + 12
// => 24

12 + 12n
Uncaught TypeError: Cannot mix BigInt and other types, use explicit conversions

Today we suggest opting into BigInt support with a custom cast function. We could also add a {bigint: true} setting to the config object used to create the connection to build this into the default casting function. That way the client app can choose to enable it and handle the values accordingly without requiring a breaking change.

switch (field.type) {
case 'NULL':
return null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be an easy commit to merge on its own.

Comment on lines -329 to -333
if (value === '' || value == null) {
return value
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need this check for null values. The switch below acts on the type of the column but not the value returned for each row.

@mattrobenolt
Copy link
Member Author

Is there precedent for something like the normal mysql driver here? I think we're choosing a set of tradeoffs that are just different tradeoffs currently.

I'd personally prefer, if we're going to introduce a config option, that we do bigint by default, but let them opt into numbers or something else that's going to lose precision.

But I get there's a difference between the data columns, but there's the two uint64's on the actual ExecutedQuery struct that make sense to be a bigint to avoid losing any precision. Arguably, insertId being a string is equally difficult since it'd likely need to be converted into bigint anyways, or a number and lose precision?

I'm not sure, I just feel like JS is a mess with this. Open to just what the community would recommend doing, I assumed we didn't use bigint since it wasn't a thing.

@mattrobenolt
Copy link
Member Author

Attempting to serialize BigInt values will throw. However, if the BigInt has a toJSON() method (through monkey patching: BigInt.prototype.toJSON = ...), that method can provide the serialization result. This constraint ensures that a proper serialization (and, very likely, its accompanying deserialization) behavior is always explicitly provided by the user.

Seems it's documented and well understood how to provide serialization for a BigInt if someone needs to just convert to JSON?

@janpio
Copy link

janpio commented Mar 31, 2023

From experience at Prisma I can tell you that many users will still be confused by that and open issues - and most importantly attribute the error message to you. (Very often the error message is also 3 levels above you, in something much closer to the frontend, so helping the user is also pretty hard sometimes.)

@iheanyi
Copy link
Member

iheanyi commented Jul 24, 2023

I don't think we're going to implement this. Gonna close this one out for 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

Successfully merging this pull request may close these issues.

None yet

4 participants