Skip to content

Postgres infinity timestamps#91

Closed
mnemitz wants to merge 5 commits intomasterfrom
postgres-infinity-timestamps
Closed

Postgres infinity timestamps#91
mnemitz wants to merge 5 commits intomasterfrom
postgres-infinity-timestamps

Conversation

@mnemitz
Copy link
Copy Markdown
Collaborator

@mnemitz mnemitz commented Apr 24, 2018

PostgreSQL supports the value of 'infinity' for the data types timestamp and timestamptz. The Postgres driver for NodeJS reads this value as Infinity of type Number. This change allows schemats to interpret this value by expanding the type mapping for timestamps to include the value Infinity.

@mnemitz mnemitz requested a review from crispmark April 24, 2018 15:53
@abenhamdine
Copy link
Copy Markdown
Contributor

Perhaps I miss something obvious here, but is there really a Infinity type in Typescript ?
I thought it's just one possible value of type number but not a type (see microsoft/TypeScript#9407 (comment) for rationale).

If I use Infinity in VS Code, I get error 'Name not found'

@xiamx
Copy link
Copy Markdown
Contributor

xiamx commented Apr 24, 2018

Please add/modify integration test. During this part, it may be revealed that the generated ts file with Infinity type does not compile, as pointed out by @abenhamdine

@mnemitz
Copy link
Copy Markdown
Collaborator Author

mnemitz commented Apr 24, 2018

@abenhamdine The Infinity name refers to the value Infinity which has type Number. In NodeJS this value is immediately interpretable, so it is no different than specifing Date | null (or even Date | 'mycustomvalue') for example I thought this was true, I was wrong. Here is some documentation of the infinity timestamp value in Postgres itself: https://www.postgresql.org/docs/9.6/static/datatype-datetime.html (see 8.5.1.4. Special Values)

@xiamx I updated the expected values in osm.ts and osm-camelcase.ts to expect the correct type (tests compiled and passed locally). Is there another file I should be changing?

Thank you both!

@mnemitz mnemitz closed this Apr 24, 2018
@mnemitz mnemitz reopened this Apr 24, 2018
@mnemitz
Copy link
Copy Markdown
Collaborator Author

mnemitz commented Apr 24, 2018

@abenhamdine You're right, I get this error when trying to build a typescript file outside of the project, but interestingly it compiled and passed just fine within the schemats project. I will continue looking into this so we can figure out the best way to cover this case :)

@xiamx
Copy link
Copy Markdown
Contributor

xiamx commented Apr 25, 2018

This is a guess:

It's possible that certain installed type definitions, for example @types/node might expose global type aliases such as type Infinity = number, which allow such compilation to be successful within the schemats project.

@mnemitz
Copy link
Copy Markdown
Collaborator Author

mnemitz commented Apr 25, 2018

Something like that yeah, I'm still surprised that Infinity is disallowed as a literal type (for NaN it makes sense but Infinity seems more useful). On my side I managed to work around this constraint by adjusting the types in my project (creating a custom (Infinity as any) as Date value worked). Still it would be nice if we could find a solution in schemats to cover this case, but I can't think of anything besides mapping it to Date | Number. If that option seems good to you I'm happy to commit the change to this PR, otherwise feel free to close it if you prefer.

@abenhamdine
Copy link
Copy Markdown
Contributor

but I can't think of anything besides mapping it to Date | Number. If that option seems good to you I'm happy to commit the change to this PR, otherwise feel free to close it if you prefer.

Indeed, actually, you cannot use better typing than number since typescript doesn't provide a narrower type.

Otherwise, I don't think mapping sql timestamp to Date | number is a good idea, because it will likely break types assertions in user code, for sake of just handling a very rare case.
I only can think of this behaviour behind a flag.

@mnemitz mnemitz closed this Aug 6, 2018
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.

3 participants