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

Why is Client.querys return type not a generic ResultIterator<T>? #66

Closed
nitwhiz opened this issue Jun 30, 2023 · 11 comments
Closed

Why is Client.querys return type not a generic ResultIterator<T>? #66

nitwhiz opened this issue Jun 30, 2023 · 11 comments

Comments

@nitwhiz
Copy link
Collaborator

nitwhiz commented Jun 30, 2023

Client.query returns a ResultIterator<Value> (and Client.query does not take any type arguments). Is there a specific reason there is no optional type parameter passed down to Client.query and Client.execute?

How to type the results of Client.query?

@malthe
Copy link
Owner

malthe commented Jun 30, 2023

I think the simple answer is that I don't know how to do that. I remember seeing a project where someone did use types in the way you're suggesting, but I'm not sure how it would work. PostgreSQL basically returns with a list of computed types for any particular query and that's what you'll get.

In a more advanced language such as Rust, it would be much more easy I think to specialize the result to a list of expected types and then return a result (okay or error) depending on whether those types matched the actual database response.

Not sure if that's possible with TypeScript – but the language keeps growing ...

@nitwhiz
Copy link
Collaborator Author

nitwhiz commented Jun 30, 2023

The first idea that comes to mind is "brute-casting" the result to the specified type parameter T. As typescript is compile-time only, there is no real way to ensure runtime type checks in a lean way. (That's a wild and interesting idea though!)

IMO that's good enough though. I can only speak for myself, but the only reason I went for ts-postgres instead of pg as dependency was that I hoped for exactly this.

To be at least somewhat aware of the contingency of the runtime types, we could wrap the specified type parameter into a Partial<T>.
This way, the ResultRow<T> from a Client.query<X> would become a ResultRow<Partial<X>>. If the column is not in the result set for some reason, it should be reflected as undefined. Otherwise it has most probably the type X[prop]. So the user should/has to check the result to make the type check happy for their code.

This of course only really works for the primitive types string and number (?). But that's better than nothing and having to cast the results yourself.

How do you think about this?

@malthe
Copy link
Owner

malthe commented Jul 4, 2023

The challenge is that the return type is erased at runtime, unless you use some of the reflection runtime support solutions, see for example https://typescript-rtti.org/.

I know that there's an ORM built-in to https://deepkit.io/ as well – perhaps this is more to your liking.

Even without the return type capabilities you're asking for, ts-postgres does have a number of features that at least set it apart at the original time of writing, including being written in TypeScript itself!

@nitwhiz
Copy link
Collaborator Author

nitwhiz commented Jul 4, 2023

I'll check these out, thanks for the pointer.

You just linked the README of ts-postgres, where do I have to look?

@malthe
Copy link
Owner

malthe commented Jul 4, 2023

@nitwhiz what I meant was the list of features listed right there on the front page – I don't know how things have evolved in the other drivers, but some of those features just weren't there when I set out to write this library.

@nitwhiz nitwhiz closed this as completed Jul 12, 2023
@malthe
Copy link
Owner

malthe commented Nov 22, 2023

For starters, we could return a ResultIterator<T = Value[]>.

See also #76.

@malthe malthe reopened this Nov 22, 2023
@nitwhiz
Copy link
Collaborator Author

nitwhiz commented Nov 26, 2023

Yes that's what I was going for.

@malthe
Copy link
Owner

malthe commented Nov 27, 2023

As far as I can tell there are two options:

  1. ResultIterator<T> and ResultRow<T> (both now generic on the query result object) would provide map(): T[] and map(): T methods, respectively. The default will be T = {[key: string]: Value}, i.e. a generic object.

    With this design, the README example would read:

    const result = client.query<{message: string}>(
        "SELECT 'Hello ' || $1 || '!' AS message",
        ['world']
    );
    
    for await (const row of result) {
        const item = row.map();
    
        // 'Hello world!'
        console.log(item.message);
    }

    The motivation for requiring the map() call here is that there's some overhead in mapping the data array to an object.

    This also does not break compatibility; it could be a stepping stone as well while considering a bigger change.

  2. Alternatively, we simply map the result row to an object to begin with, obviating the needs for a get method (and removing this method altogether).

    This is perhaps better in line with the principle of least surprise, but wouldn't provide an easy means of giving access to the column names and data arrays.

In both cases, we could consider wrapping T as Readonly<T> in the result types.

@nitwhiz
Copy link
Collaborator Author

nitwhiz commented Dec 5, 2023

How about row being treated as the Readonly<T> type? (I think you're going for this with 2)

// result: ResultIterator<Readonly<{ message: string; }>>
const result = client.query<{ message: string; }>(
    "SELECT 'Hello ' || $1 || '!' AS message",
    ['world']
);

for await (const row of result) {
    // row: Readonly<{ message: string; }>
    // 'Hello world!'
    console.log(row.message);
}

This would be the simplest approach for the user, just as you pointed out.

@malthe
Copy link
Owner

malthe commented Dec 5, 2023

@nitwhiz originally, the row being ResultRow<T> and not simply T itself was for compatibility with https://node-postgres.com/.

There was another concern as well, but perhaps not a particularly valid one: providing names and values (as arrays), rather than objects should be a more compact representation in memory.

The get method now uses a lookup table to quickly return a value for a given column name, but some users will perhaps just want to unpack the rows property. Meanwhile, I don't think there's a way in TypeScript to meaningfully type the rows based on an interface (since the keys aren't ordered).

In that case, T would instead have to be a tuple type, e.g.

type Book = [["title", string], ["author", string], ...]

Perhaps an interface can be derived from such a type.

But it might be possible to provide an overload to query and friends:

const result = await client.query<[string, string, number]>(...);

This would work the same, but map would return a Record<string, any> while rows would be typed correctly.

@malthe
Copy link
Owner

malthe commented Dec 6, 2023

@nitwhiz I have opened a new issue to carry over the second proposal.

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

No branches or pull requests

2 participants