search

brianc / node-pg-types

Type parsing for node-postgres
268 stars 55 forks source link

fix: Normalize text protocol handling of numeric and numeric[] types #91

Closed sehrope closed 5 years ago

sehrope commented 5 years ago

NOTE: This does not fix the binary protocol, only the text one.

sehrope commented 5 years ago

Closing this out as it was included in #94

charmander commented 5 years ago

… as one commit labelled “Add linter (standard)”, https://github.com/brianc/node-pg-types/commit/ee2225276674357027de4fef6a2052353f902360. It’s kind of hidden in there. Is it too late to split them? @bendrucker

bendrucker commented 5 years ago

I haven't published a release to npm. It's been about 24h so I think it's worth it to rewrite the history to split it. Will do now.

bendrucker commented 5 years ago

Did some history rewriting to fix this. Anything else we should be doing around numbers in a breaking release here? It would be nice to give people the options to get BigInts back for this stuff and push towards making this work the way everyone expects (#78) now that it's possible.

bendrucker commented 5 years ago

Although I guess that really only relates to count/int behavior, not floats. But I'd like to still include a breaking change for #78 (enabled by default if supported) in the same major that releases this PR.

sehrope commented 5 years ago

@bendrucker Thanks for the history cleanup!

I'm a -1 on feature detection changing results. If we're still going to support Node v8 until it's out of LTS then I say release one semver major with the current round of changes and another in Jan 2020 when Node v8 is EOL.

For the record, I'm for the BigInt change itself, just seems brittle to have a node version change the output type of a column. Anything that uses it would have to perform it's own version detection (even the tests for this module).

yocontra commented 5 years ago

@sehrope Did you do a follow-up PR? Can't seem to find one.

sehrope commented 5 years ago

@contra Follow up for what?

The text protocol fix for numeric/numeric[] was added in https://github.com/brianc/node-pg-types/commit/afead5b156c179364d173965bae2d661beb2230f (it's this PR after some rewriting to fix me accidentally checking in the packed up module). @charmander has a separate branch to redo the binary parsing but it hasn't been submitted as a PR yet.

yocontra commented 5 years ago

@sehrope Thanks, exactly what I was looking for.