It seems like pg-copy-streams has a lower minimum TS version causing the tests to fail, should I update its minimum version?

@HoldYourWaffle Thank you for submitting this PR!

🔔 @pspeter3 @fluggo @bradleyayers @mateuszkrupa @aleung @mainnika @asmarques - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@HoldYourWaffle The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

Seems like #37689 is a reduced implementation of this since it only types the return value. I'm not 100% sure what the additional RowBase interface is supposed to be though.

Interesting to see that someone had this idea too. I've had this changes running in production for a while now, I honestly just forgot to send a PR out 😅

RowBase was there to constraint the generic types you can pass around. For instance, we can tell at compile time that query<number>(...) is a bug, as no SQL query can be written in the context of pg that results in a number (nor an array of numbers) being returned.

That makes a lot of sense, I'll incorporate something similar in this PR.
Would it make sense to add a similar constraint type to the I generic?

I believe that for I is enough with enforcing that it must extend any[]. You could possibly go further and say something like extends (null | boolean | number | string | Date | Uint8Array)[] or such, but you never know which types might be returned by the query() methods due to node-postgres' support for custom parsers.

Edit On a second thought, I don't see the value on having the I generic type: embedded SQL queries are unsafe on themselves (i.e. the types provided for the values aren't going to be verified against those supported by the query); and the types can't be defined once and used for multiple sets of values either, I believe, which makes the type declaration quite redundant. It feels to me as saying let s: string = '' (as opposed to just let s = '') when you are never going to use s again. Could you please try to elaborate on why you think I could be useful?

but you never know which types might be returned by the query() methods due to node-postgres' support for custom parsers.

I'm not sure what the return type of query has to do with I, I is for the input parameters.

and the types can't be defined once and used for multiple sets of values either, I believe, which makes the type declaration quite redundant.

I'm not sure what you mean by this, could you clarify? What do you want to re-use?

It feels to me as saying let s: string = '' (as opposed to just let s = '') and never using s. Could you please try to elaborate on why you think I could be useful?

In a way it's definitely like let s: string = ''. I already put my rationale for adding it in my original post:

The I parameter (the inputs) might be too repetitive/over-safe for some people, which is why it comes after the (more useful) returntype generic.
For my use-case I'm automatically generating parts of these method calls, and an extra sanity check is very much welcome in that case.
It's also a very welcome safeguard when refactoring database types, although that probably doesn't happen too often for most people.

To elaborate on my use-case, I have a setup where database type definitions are generated for me. So I'm writing my queries like this:

await database.query<void, [UserFields.id]>(
    'UPDATE "user" SET date_last_seen = now() WHERE id=$1',
    [ user.id ]
);

As you see I'm not literally reusing the same type twice. The user variable could come from anywhere and its internal representation might differ from the database, this is where the extra type-check comes in handy.
This is of course a very simple example, but it's enough to demonstrate my use-case.
I recognize that not everyone will find this extra check useful, which is why it's 100% opt-in. Embedded SQL queries indeed inherently type-unsafe, but with this extra check hopefully a little less so.

I'm not sure what the return type of query has to do with I, I is for the input parameters.

I intended to refer to custom serializers, as opposed to parsers. Sorry for the confusion.

I'm not sure what you mean by this, could you clarify? What do you want to re-use?

If you could somehow declare the types of the values once and have them checked against multiple sets of values, I thought that would somehow be a possible justification for declaring them, but it doesn't seem to be the case. To illustrate what I meant, see the following example which allows exactly that, although through an extra function:

// Types declared once
function getSomething(values: [string, number]) {
  pg.query(`select * from something where s = $1 and n = $2`, values)
}

// Type-checking performed multiple times
getSomething(['0', 0])
getSomething(['1', 1])

To elaborate on my use-case (...)

Thanks for the additional explanation. I still find it somewhat weird to specify the types of your own input, over which you have complete control. But I'm fine with the change.

Thanks for the additional explanation. I still find it somewhat weird to specify the types of your own input, over which you have complete control.

But I'm fine with the change.

That's great to hear 😄 I think the bot wants you to push some approve button somewhere in that case.
In the meantime, how should I handle the travis failure? Should I raise the minimum typescript version for pg-copy-streams?

@pspeter3 What do you think of these changes?

@inad9300 Can you double check for me if I implemented your RowBase-like constraint correctly?

Updated numbers for you here from 033bac5:

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

@HoldYourWaffle The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

QueryResultRow should be applied to QueryResult and not to QueryResultArray, as the latter represents (mostly) a matrix of primitives, and QueryResultRow models a key-value structure. As a basic check that the code is correct, I believe the expression extends QueryResultRow[] should appear nowhere in the code.

Whoops that's a pretty dumb mistake, no idea how I missed that. I think I've fixed it now, but it's too early for me to tell...

Updated numbers for you here from e91f271:

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

@HoldYourWaffle The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@inad9300 I have addressed your concerns.

I have also updated the minimum typescript version for @types/pg-copy-streams to avoid travis failing.

@HoldYourWaffle The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Updated numbers for you here from 8db1e4b:

pg-copy-streams/v1

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

pg-ears/v1

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

pg-large-object/v2

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

pg/v7

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

Updated numbers for you here from 479677f:

pg-copy-streams/v1

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

pg-ears/v1

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

pg-large-object/v2

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

pg-pool/v2

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

pg-query-stream/v1

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

pg/v7

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

Finally got travis to pass.
@inad9300 I think the only thing left now is for you to press some kind of "approve" button somewhere

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

Thanks for the contribution!

I just published @types/pg@7.11.1 to npm.

I just published @types/pg-ears@1.0.1 to npm.

I just published @types/pg-pool@2.0.1 to npm.

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: not applicable
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header. (not applicable)
• If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

---

I generified the query methods as well as the corresponding interfaces. R will be the return type of the query, I a tuple for the parameters.

A usage example:

interface Person {
    name: string;
    naughty: boolean;
}

/* 
   The types are defined here:
   - The return type R will be the 'Person' interface defined above
   - The parameters (I for input) will be a string and boolean (in order)
*/
client.query<Person, [string, boolean]>(
    'SELECT * FROM user WHERE name = $1 AND naughty = $2',
    [ 'Johnny', true ], // this array will be typechecked
    (err, result) => console.log(result.rows[0].name) // this will now have a 'string' type
);

These additional type checks are aimed at users who have interfaces for their database types (for example auto-generated by schemats).
It is 100% opt-in and backwards compatible with the use of generic defaults added in TS 2.3.

The usefulness R parameter (for the return type) pretty much speaks for itself, it enables intellisense/typechecking without the use of annoying type assertions.

The I parameter (the inputs) might be too repetitive/over-safe for some people, which is why it comes after the (more useful) returntype generic. For my use-case I'm automatically generating parts of these method calls, and an extra sanity check is very much welcome in that case. It's also a very welcome safeguard when refactoring database types, although that probably doesn't happen too often for most people.

Because these generics are meant to check the parameters supplied directly the no-unnecessary-generics rule doesn't apply here. The dtslint page says:

Generic type parameters allow you to relate the type of one thing to another; if they are used only once, they can be replaced with their type constraint.

The emphasized part doesn't apply here, because the whole idea of these generics is to have users (optionally) manually provide an extra layer of security by specifying their own types. any is a perfectly valid type constraint here, the generics enable users to narrow it down to their own types if they so wish.