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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for utility types #92

Closed
karlhorky opened this issue Oct 26, 2022 · 12 comments 路 Fixed by #94
Closed

Support for utility types #92

karlhorky opened this issue Oct 26, 2022 · 12 comments 路 Fixed by #94
Labels
bug Something isn't working

Comments

@karlhorky
Copy link
Collaborator

karlhorky commented Oct 26, 2022

Hi @Newbie012 , hope you are well! 馃憢

Is your feature request related to a problem? Please describe.

It would be great to be able to use generic utility types such as Pick and Omit with existing types to be able to transform existing types instead of copying them throughout the codebase:

type User = {
  id: number;
  username: string;
}

export async function getUserByUsername(username: string) {
  const [user] = await sql<Pick<User, 'id'>[]>`
    SELECT
      id
    FROM
      users
    WHERE
      username = ${username}
  `;
  return user;
}

Right now, this leads to a confusing error message:

Query has incorrect type annotation.
	Expected: null[]`
	Actual: { id: number; }[]
eslint @ts-safeql/check-sql

Describe the solution you'd like

It would be great if the utility types worked

Describe alternatives you've considered

Workaround:

Use the literal object type instead of the utility type (decreasing maintainability):

export async function getUserByUsername(username: string) {
  const [user] = await sql<{ id: number }[]>`
    SELECT
      id
    FROM
      users
    WHERE
      username = ${username}
  `;
  return user;
}

Additional context

--

@Newbie012
Copy link
Collaborator

It seems like TypeScript doesn't expose its API if the type is identical to another one, so I created a question in Stack Overflow

Until then, I'll write my own isIdentical function, it should initially support:

  1. Type Literal
  2. Type Reference
  3. Pick
  4. Omit
  5. Type Intersection

Once it's complete, I'll be able to close #93 as well.

@karlhorky
Copy link
Collaborator Author

@orta mentioned that there are some prior art for testing types here: effectivetypescript.com/2022/05/28/eslint-plugin-expect-type

@Newbie012
Copy link
Collaborator

Newbie012 commented Oct 28, 2022

I did a little digging at the source code of eslint-plugin-expect-type (the one that is in the article), and the "magic-souce" is the following lines:

cosnt qi = languageService.getQuickInfoAtPosition(sourceFile.fileName, tad.getStart());
const actual = qi.displayParts.map((dp) => dp.text).join("");

The thing is, It's very accurate when you need to get the type definition of a reference.
It's so accurate that when you want to know what fn<Pick<Person, "name">>() is, it returns type Pick<T, K extends keyof T> = { [P in K]: T[P]; }.

But, if I assign that type to a new type (type PickPersonName = Pick<Person, "name">) it will work.

image

@karlhorky
Copy link
Collaborator Author

karlhorky commented Oct 28, 2022

Interesting. Usually I assign to an alias already, but I guess at some point, some user will want it inline... if you can't find a quick way to do it now, maybe could be a documented "known issue" + open GitHub issue and then maybe implementation considered for later in the future?

@Newbie012
Copy link
Collaborator

I think I'll go with the following approach:

  • If it's a type literal, keep the current behavior
  • If it's a type reference (that is not a utility type), use the language service.
  • If it's a utility type, go with the poor implementation I wrote above. It should cover most of the cases.

@Newbie012 Newbie012 added the bug Something isn't working label Oct 28, 2022
@Newbie012
Copy link
Collaborator

Another problematic approach with getQuickInfoAtPosition:

type Starship = { id: number; name: string; captain_id: number | null; };
type Type = Pick<Starship, "captain_id" | "id"> & { x: number; }

function fn<T>() {};
fn<Type>();
   // ?^ type Type = Pick<Starship, "captain_id" | "id"> & { x: number; }

@karlhorky
Copy link
Collaborator Author

karlhorky commented Oct 29, 2022

  1. Does it matter here that ? comes before ^ in this comment? In the other example, the ^ was before the ?
  2. Does the _ "type resolver" type trick used in Yup (or anything similar) help here?

@Newbie012
Copy link
Collaborator

  1. Typo, should be ^?
  2. It's impossible to look for a type that doesn't exist in the file (physically).

@Newbie012
Copy link
Collaborator

I eventually found a better solution that solves all of the scenarios

@karlhorky
Copy link
Collaborator Author

karlhorky commented Oct 30, 2022

Great! I'll try out the new changes in the release @ts-safeql/eslint-plugin@0.0.21!

What's the quick summary? (just for anyone else who is trying to do something similar)

I imagine it has to do with these getTypeProperties and toInlineLiteralTypeString functions exported in packages/eslint-plugin/src/utils/get-type-properties.ts?

@Newbie012
Copy link
Collaborator

Correct. There's no quick way of doing this. It depends on the ts.TypeNode kind

karlhorky added a commit to upleveled/security-vulnerability-examples-next-js-postgres that referenced this issue Oct 31, 2022
@karlhorky
Copy link
Collaborator Author

@ts-safeql/eslint-plugin@0.0.21 works great, thanks! 馃檶

gruvector pushed a commit to gruvector/security-vulnerability that referenced this issue Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants