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

feat: Typed sql overload functions #520

Merged
merged 10 commits into from
Sep 29, 2023

Conversation

JesseVelden
Copy link
Contributor

Hi @adelsz, I hope you’re doing well! Here is my shot at implementing the typed SQL tag functionality as also mentioned in #495.

I think the best developer experience would be to generate one file with all the sql overload functions found in a project, so you would only need to import the sql function from one place (also would be the same as in the GraphQL codegen).

It involved quite some changes to the core cli package as it needs to generate one file:
We need to wait for the results of all the include files before generating the file with the sql overload functions. I made new transfomer classes, where it can handle generating the overload functions from several files into one file. (Of course, this can be programmed in a thousand different ways, so feel free to suggest/ make any changes.)

The TypeAllocator class also needed to be serializable because of getting the results out of the Piscina pool, so I’ve just added an extra method to convert it to a simple object for later use.

For the watch mode, I’ve made a object cache storing its filename and the generated type declarations object.Then when a file updates, it would use the cache of all the previously generated types, add the just generated types and generate the file again.

For the configuration: I have added another transformation config for this functionality. In theory it could have been added to the default ts transformation, but then you wouldn’t be able to decide one, the other or both at the same time. That would have been a braking change. So now, an example would be (I now use in an internal SolidJS project):

 {
      "mode": "ts-typed-sql-tag",
      "include": "{routes,components,server}/**/*.{ts,tsx}",
      "functionName": "sql",
      "emitFileName": "sql/index.ts"
 }

Maybe we can just rename the mode to something simpler 😁

A outstanding issue:
Extra white spaces/ new lines at the beginning and a semicolon at the end are ignored by the parser, but are needed for the overload functions, as otherwise they wouldn't match the input. We could simply return the full query including extra whitespaces and semicolons in the parseTypescript.ts file, or we could modify the actual parser grammar to include it. I think the former would be sufficient and the easiest.

Let me know what you think!

@water-a
Copy link
Contributor

water-a commented May 21, 2023

@adelsz this would be crazy dx boost, if you ever get the chance to review, please review this one!!!


private contentStart = `/* eslint-disable */\nimport { ${this.transform.functionName} as sourceSql } from '@pgtyped/runtime';\n\n`;
private contentEnd = [
`export function ${this.transform.functionName}(s: string): unknown;`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When testing locally; I found I had to remove this line to get type-completion to work properly

@adelsz adelsz self-requested a review September 26, 2023 21:52
@adelsz
Copy link
Owner

adelsz commented Sep 29, 2023

Thanks for the great PR @JesseVelden!

I fixed some bugs regarding incorrect import paths generated for some file configurations and made sure that watch mode is more robust to parsing errors.

Noted about extra spaces breaking the type matching, we can tackle that later.

Merging this as an experimental/undocumented feature for now. Once it stabilizes a bit we should make this approach the default. One improvement I would like to add before this is documented is to write the overloaded sql function into node_modules/@pgtyped/runtime by default. Similar to how Prisma is doing it, completely hidden from the code tree of our users.

@adelsz adelsz merged commit e5f920e into adelsz:master Sep 29, 2023
2 checks passed
@JesseVelden
Copy link
Contributor Author

Thanks for your input as well @adelsz! I also like the idea of putting the types into node_modules/@pgtyped/runtime as well,.

What would be a good place to keep the discussion going on these last topics? A new issue for the improvements?

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.

4 participants