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 batch method generic, similiar to Promise.all #14

Merged

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Dec 19, 2020

The goal of this PR is to improve the type definitions for the batch method so that it behaves more like Promise.all.

Currently, when batch resolves it always resolves to an array with type any[] which means we've lost all type information about the values sent in.

This PR aims to preserve the type information for arrays and tuples of length 2 through 10.

Now:

import Spex from './typescript/spex';

async function testTuples(s: Spex.ISpexBase) {
  const result = await s.batch(['1',Promise.resolve(2)]);
  const [a, b]: [string, number, boolean] = result;

  // IArrayExt duration value still works
  const duration: number = result.duration;

  // the above should behave same for tuples of length 2 through 10.
  // With more than 10 values, the result array items would all have
  // type string | number | boolean
}

async function testArbitraryLength(s: Spex.ISpexBase, toResolve: Promise<number | string>[]) {
  const result: Spex.IArrayExt<number | string> = await s.batch(toResolve);
  const duration: number = result.duration;
}

Hardcoding 9 type definitions for tuples of length 2 through 10 is not particularly elegant. I do not know if there is a better way, but this seems to follow the typescript definitions of Promise.all.

@coveralls
Copy link

coveralls commented Dec 19, 2020

Coverage Status

Coverage remained the same at 99.351% when pulling 169abe3 on ChristopherChudzicki:improve-batch-typings into 8166945 on vitaly-t:master.

@ChristopherChudzicki
Copy link
Contributor Author

@vitaly-t First, thanks for pg-promise. It's great!

One note about this PR: In my opinion, this definitely improves the type definitions for batch. But I guess it's also a breaking change since typings that may have passed beforehand may potentially not pass now. But I guess they should not have passed beforehand, so maybe that's not a problem? I am not too familiar with how type improvements usually get versioned. 🤷

The goal of this commit is to improve the type definitions for the `batch` method so that it behaves more like `Promise.all`.

Currently, when `batch` resolves it always resolves to an array with type `any[]` which means we've lost all type information about the values sent in.

This commit aims to preserve the type information for arrays and tuples of length 2 through 10.

Now:

```ts
import Spex from './typescript/spex';

async function testTuples(s: Spex.ISpexBase) {
  const result = await s.batch(['1',Promise.resolve(2)]);
  const [a, b]: [string, number, boolean] = result;

  # IArrayExt duration value still works
  const duration: number = result.duration;

  // the above should behave same for tuples of length 2 through 10.
  // With more than 10 values, the result array items would all have
  // type string | number | boolean
}

async function testArbitraryLength(s: Spex.ISpexBase, toResolve: Promise<number | string>[]) {
  const result: Spex.IArrayExt<number | string> = await s.batch(toResolve);
  const duration: number = result.duration;
}
```

Hardcoding 9 type definitions for tuples of length 2 through 10 is not particularly elegant. I do not know if there is a better way, but this seems to follow the [typescript definitions of Promise.all](https://github.com/microsoft/TypeScript/blob/2428ade1a91248e847f3e1561e31a9426650efee/src/lib/es2015.promise.d.ts).
@vitaly-t
Copy link
Owner

Use of batch within pg-promise is more of a legacy these days, it is not really needed anymore. I am keeping it there mainly for compatibility sake. ES7 syntax is way better to use.

@ChristopherChudzicki
Copy link
Contributor Author

ChristopherChudzicki commented Dec 19, 2020

In an existing codebase with significant usage of batch, getting value from Typescript would be easier if batch preserved types correctly.

(I do think batch offers some advantages over simple async/await; the queries themselves might not be parallelized since they share a single connection, but at least the communication between backend and database server can be parallelized. And a dynamic number of calls be used. The multi method seems like a good alternative in both respects, but also seems less flexible. We've had success with reusable functions f1, f2, ... that make database calls + do some processing in js; something along the lines of t.batch([f1(), f2(), ...]) seems infeasible with multi.)

Anyway, batch, async/await, multi discussion aside, usage of batch exists and accurate typings seem helpful. I hope you'll consider them, but we can always use the typings locally.

Thanks, again, for pg-promise!

@vitaly-t vitaly-t merged commit 0d708c1 into vitaly-t:master Dec 20, 2020
@vitaly-t
Copy link
Owner

Thanks! This has been released in v3.1.0.

@vitaly-t
Copy link
Owner

vitaly-t commented Dec 20, 2020

pg-promise started using it from v10.8.3.

@vitaly-t
Copy link
Owner

vitaly-t commented Dec 20, 2020

In v3.2.0 made it even more strict, defaulting to unknown for single-template batch.

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

3 participants