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

Postgres infinity timestamps #91

Closed
wants to merge 5 commits into from
Closed

Conversation

mnemitz
Copy link
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
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
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
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
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
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
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
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