-
Notifications
You must be signed in to change notification settings - Fork 25
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
int8/bigint/bigserial: number vs BigInt #152
Comments
Looking at The idea was for every data type to have a configurable type with a sane default e.g. |
I was just about to file a separate bug for the This is how to tell "pg" to return import pg from "pg";
import pgTypes from "pg-types";
const client = new pg.Client({
connectionString: "postgres://localhost:5432/anrok?sslmode=disable",
types: {
getTypeParser(typeId: pgTypes.TypeId, format: 'text' | 'binary'): any {
if (typeId === pgTypes.builtins.INT8) { // also used for "bigint" and "bigserial"
assert(format === 'text');
return BigInt;
} else {
return pgTypes.getTypeParser(typeId, format);
}
}
}
} as any); // The "any" is necessary because "@types/pg" doesn't support the "types" parameter yet Because "pg" defaults to The first option I thought of was to provide additional
However, the problem now is that Mammoth has to parse the string. But this would require Mammoth to transform every row returned by "pg", which isn't ideal. An alternative is to match what "pg" allows -- a global change for the entire schema, e.g.:
I'm not familiar with Mammoth internals, so I'm not sure exactly how this would work :-P |
That said, maybe the "best" option is not worth the effort/complexity. A simpler option:
|
Thank you for your thoughts on this. I just applied a fix in 86b6655 to default It seems I documented this including the bigint workaround at https://mammoth.tools/defining-tables#data-types. I also added a suggestion on how to create a reusable data type so you don't have to specify e.g. |
Thanks! This is a sort of a minor point, but the example you added to the docs mutates global state. It's definitely the easiest option, but for people who (like me :-) who prefer to avoid global state if possible, maybe it's worth pointing to the per-Client/per-Pool Feel free to ignore if you prefer. |
I agree. I'll update this in the docs. |
Thanks for adding
int8
/bigint
/bigserial
support in efdb893.One problem: JS
number
can't losslessly represent all the values in an 64-bit integer.BigInt
, which can represent allint8
values.Suggestion: expose two
int8
types, one fornumber
and one forBigInt
.int8AsJsBigInt
andint8AsJsNumber
so the user knows what they're getting in to.int8
andint8AsJsNumber
. Node 10 is the oldest actively-supported version of Node, so defaulting to BigInt might be reasonable. And the user can opt-in tonumber
if they want.The text was updated successfully, but these errors were encountered: