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

Make Result iterable #2861

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make Result iterable #2861

wants to merge 1 commit into from

Conversation

vitaly-t
Copy link
Contributor

@vitaly-t vitaly-t commented Nov 14, 2022

Make Result automatically iterable, so one can iterate through rows without having to access rows explicitly:

client.query('select...', (err, res) => {
    // below we can use just "res", not "res.rows"
    for(const a of res) {
        console.log(a); // print the row
    }
})

This also simplifies data processing when you want to process it as just iterable, all the way, not as an array.

Make `Result` automatically iterable, so one can iterate through data without having to access `rows` explicitly:

```js
client.query('select...', (err, res) => {
    // below we can use just "res", not "res.rows"
    for(const a of res) {
        console.log(a); // print the row
    }
})
```
@vitaly-t
Copy link
Contributor Author

vitaly-t commented Nov 14, 2022

In an afterthought, it would make the library perform better, if rows array is retired, and replaced with an iterator, because it doesn't make sense executing all that data-type conversion unless it is actually used. In many cases, particularly with large data results, the client ends up not ever using the data, rather a small portion of it, like with paging, for example.

This PR above is a first step toward that. For now, it just forwards into rows values, but later it may implement real iteration through data, with dynamic data-type conversion, on-demand.

The bottom line, modern development practices gravitate toward iterables versus pre-built lists.

vitaly-t added a commit to vitaly-t/node-pg-native that referenced this pull request Nov 14, 2022
This is to align with [this main-driver change](brianc/node-postgres#2861). The two PR-s should be merged at the same time.
@vitaly-t
Copy link
Contributor Author

vitaly-t commented Nov 14, 2022

@charmander
Copy link
Collaborator

I don’t think this is worth changing now.

  • Multi-statement queries return an array of results that is also iterable, which could be confusing.
  • res.rows, {rows}, or a wrapper function are easy to write.
  • People will often want to use array methods, which aren’t available on the plain iterable anyway.

It’s nice when there’s just one way to do it.

@vitaly-t
Copy link
Contributor Author

This caters for all coding styles, without any change in functionality. It also opens a path to changing types conversion, to become dynamic. There are lots of libraries these days that handle iterables well.

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.

None yet

2 participants