Skip to content

[Typescript] Improve sql() types #85

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.

Sign up for GitHub

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

Jump to bottom
Closed
michael-land opened this issue Jun 20, 2020 · 13 comments
Closed

[Typescript] Improve sql() types #85

michael-land opened this issue Jun 20, 2020 · 13 comments
Labels
enhancement New feature or request
Milestone
v2

Comments

@michael-land
Copy link

michael-land commented Jun 20, 2020 •
edited
Loading

Looks like that we cannot pass pre defined interface to sql() helper. @Minigugus

// table.genarated.ts

interface ExchangeRate {
  date: Date;
  source: string;
  target: string;
  rate: number;
}
export interface ExchangeRateSelect extends SetRequired<ExchangeRate> {}
export interface ExchangeRateInsert extends ExchangeRate {}
export interface ExchangeRateUpdate extends Partial<ExchangeRate> {}
    const data: ExchangeRateInsert[] = [];

    // not work
    const result = await sql`
      INSERT INTO exchange_rate 
      ${sql(data)}
      ON CONFLICT ON CONSTRAINT exchange_rate_pkey
      DO NOTHING`;


    // not work
    const result = await sql`
      INSERT INTO exchange_rate 
      ${sql<ExchangeRateInsert[],[]>(data)}
      ON CONFLICT ON CONSTRAINT exchange_rate_pkey
      DO NOTHING`;

    // not work
    const result = await sql`
      INSERT INTO exchange_rate 
      ${sql<ExchangeRateInsert[]>(data)}
      ON CONFLICT ON CONSTRAINT exchange_rate_pkey
      DO NOTHING`;

No overload matches this call.
  Overload 1 of 4, '(template: TemplateStringsArray, ...args: SerializableParameter[]): PendingQuery<Row[]>', gave the following error.
    Argument of type 'PendingQuery<Row[]>' is not assignable to parameter of type 'SerializableParameter'.
      Type 'PendingQuery<Row[]>' is missing the following properties from type 'SerializableParameter[]': length, pop, push, concat, and 28 more.
  Overload 2 of 4, '(...columns: string[]): Helper<string, string[]>', gave the following error.
    Argument of type 'TemplateStringsArray' is not assignable to parameter of type 'string'.
  Overload 3 of 4, '(objOrArray: HelperSerializable, ...keys: JobId[]): Helper<HelperSerializable, JobId[]>', gave the following error.
    Argument of type 'TemplateStringsArray' is not assignable to parameter of type 'HelperSerializable'.
      Type 'TemplateStringsArray' is not assignable to type '{ [index: string]: SerializableParameter; }'.
        Index signature is missing in type 'TemplateStringsArray'.ts(2769)
No overload matches this call.
  The last overload gave the following error.
    Argument of type 'ExchangeRateInsert[]' is not assignable to parameter of type 'HelperSerializable'.
      Type 'ExchangeRateInsert[]' is not assignable to type '{ [index: string]: SerializableParameter; }[]'.
        Type 'ExchangeRateInsert' is not assignable to type '{ [index: string]: SerializableParameter; }'.
          Index signature is missing in type 'ExchangeRateInsert'.ts(2769)
index.d.ts(312, 5): The last overload is declared here.

Updated

However, it works if you define these type inline or cast it.

const result = await sql`
  INSERT INTO exchange_rate 
  ${sql<{ 
       date: Date;
       source: string;
       target: string;
       rate: number;
   }[],[]>(data)}
  ON CONFLICT ON CONSTRAINT exchange_rate_pkey
  DO NOTHING`;


const result = await sql`
  INSERT INTO exchange_rate 
  ${sql<Readonly<ExchangeRateInsert>[],[]>(data)}
  ON CONFLICT ON CONSTRAINT exchange_rate_pkey
  DO NOTHING`;
@michael-land michael-land changed the title Typescript. How to type insert statement? [Typescript] Improve sql() types Jun 20, 2020
@Minigugus
Copy link
Contributor

TLDR workaround: npm remove @types/node 😆 (no joke, it really solve this issue for now!)

In order to keep postgres with 0 dependencies, I prefered not to install @types/node as dev dependencies despite using Buffer here. But I also forgot to install @types/node locally so Buffer was infered as any and my tests passed correctly, so I missed Index signature is missing in type... 😒

However, it works if you define these type inline or cast it.

Ok so I'm not the only guilty 😅 microsoft/TypeScript#15300
If you really want your interface you can wrap your type in type Noop<T> = { [k in keyof T]: T[k]; };...

I tried to get the strongest types I could to prevent unexpected SQL errors at runtime but sometimes they are too strong 😆

I'm working on a fix, what about this one? (@ts-expect-error indicate the line should fail)

@ghost ghost mentioned this issue Jan 14, 2021
Closed
@porsager
Copy link
Owner

@Minigugus can this be closed after merging #84 ?

@Minigugus
Copy link
Contributor

Minigugus commented Feb 12, 2021 •
edited
Loading

@porsager The #84 didn't fixed this since it wasn't clear whether the integration of Node types should have been fixed or dropped (it seems that postgres accepts Buffer but not Unit8Array, even if Buffer extends Uint8Array).

Do you prefer to drop Node types integration and supports Uint8Array instead of Buffer, or to fix Node types integration by adding @types/node dev dependency to the project (which would break the 0 dependency principle)?

@porsager
Copy link
Owner

Ah, we should definitely just support Uint8Array :)

@porsager porsager added the enhancement New feature or request label Mar 23, 2021
@porsager
Copy link
Owner

porsager commented Jun 20, 2021 •
edited
Loading

Hmm. Switching to Uint8Array is not an option since we can't do allocUnsafe.

What's the alternative to fix this issue - adding the dev dependency?

@Minigugus
Copy link
Contributor

Switching to Uint8Array is not an option since we can't do allocUnsafe.

Why couldn't we do allocUnsafe? As explained in the NodeJS docs, Buffer extends Uint8Array and allocUnsafe is static, so const buffer: Uint8Array = Buffer.allocUnsafe(42); in TypeScript should just work (downcast). However, if you need Buffer methods from a user provided Uint8Array buffer (or any TypedArray buffer), you can still use Buffer.from(arrayBuffer[, byteOffset[, length]]) to create a new Buffer instance that share the user-provided buffer memory (no copy).

Note: only user-provided buffers matter here; there is no need to convert Buffer to Uint8Array in the internal code

@porsager
Copy link
Owner

Ah, I misunderstood then. So is this only something that needs fixing in the types?

@Minigugus
Copy link
Contributor

Minigugus commented Jun 21, 2021 •
edited
Loading

Ah, I misunderstood then. So is this only something that needs fixing in the types?

Yes, but that may also break the internal code if buffers are expected to extend Buffer (for instance bufferParam.toString('base64') (where bufferParam is a user-provided buffer passed as tagged template parameter) won't work). It's not safe to just change types in case, since there is no guaranty the JS code works with Uint8Array buffers.

@porsager
Copy link
Owner

porsager commented Jun 21, 2021 •
edited
Loading

The only place that happens is in the bytea serializer right?

I already wanted to send bytea as binary in the protocol instead of a hex string to improve performance, so that could even make it go straight through as Uint8Array.

Would that be sufficient?

@porsager
Copy link
Owner

porsager commented Jun 21, 2021 •
edited
Loading

Even so, wouldn't Buffer.from(bufferOrUint8Array).tostring('hex') in the bytea serializer be fine too?

@Minigugus
Copy link
Contributor 

Buffer.from(bufferOrUint8Array.buffer, bufferOrUint8Array.byteOffset, bufferOrUint8Array.byteLength)
      .tostring('hex')

should work, indeed (not tested). The extra parameters ensure the new buffer share the input buffer memory instead of copying it.

@porsager
Copy link
Owner

Ah, very cool! Let's go for that now then.

@porsager
Copy link
Owner

Do you want to make a PR @Minigugus ?

@porsager porsager added this to the v2 milestone Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v2
Development

No branches or pull requests
3 participants
@porsager @michael-land @Minigugus