search

brianc / node-pg-types

Type parsing for node-postgres
267 stars 54 forks source link

Simplify date handling #119

Closed charmander closed 3 years ago

charmander commented 4 years ago

A major backwards-compatibility break, unfortunately.

The binary parser’s approach of creating a local timestamp then adjusting it by its timezone offset to get the same components in UTC just doesn’t work, producing the wrong results around daylight saving transitions, for example. But even when it’s fixed to work as well as the text parser does, some times are still completely unrepresentable because they don’t exist in local time (daylight saving transitions are, again, an example of this). The more reliable way is:

charmander commented 4 years ago

Related: #50, #81, brianc/node-postgres#2154, https://github.com/brianc/node-postgres/issues/1200#issuecomment-562386989, brianc/node-postgres#993, brianc/node-postgres#783 many more.

pg’s Date serialization would need to change along with this (to act like it does with parseInputDatesAsUTC).

Any ideas on making this less hazardous for people upgrading? Or maybe this change just can’t be made at all? Opt-in with a warning for a whole major version, maybe?

bendrucker commented 4 years ago

I'm definitely for getting things correct even if there are upgrade hazards. I'm not a big believer in warnings since they tend to go unnoticed, especially if they're not on startup or install. I think a major version with multiple significant type changes (this, int8 as BigInt, and any others) is the clearest approach.

bendrucker commented 3 years ago

Forgot about this, sorry. Just merged #53, trying to collect breaking changes into https://github.com/brianc/node-pg-types/milestone/1. I think the best answer here is to collect these related changes into a single breaking release, provide clear guidance in the release notes, and move forward. Warnings and options go unnoticed by the vast majority of users.

Let me know if this PR is ready and I can merge it.

charmander commented 3 years ago

I think this PR is ready, then. I expect to open a separate one for a binary date parser soon, too.

codecov-io commented 3 years ago

Codecov Report

Merging #119 into master will increase coverage by 4.08%. The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   87.61%   91.70%   +4.08%     
==========================================
  Files           4        4              
  Lines         210      205       -5     
==========================================
+ Hits          184      188       +4     
+ Misses         26       17       -9
Impacted Files Coverage Δ
lib/binaryParsers.js 86.30% <75.00%> (+9.47%) :arrow_up:
lib/textParsers.js 98.19% <92.30%> (+0.06%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 69b5621...f1778a0. Read the comment docs.