Support Idiomatic Postgres.js

This issue has been tracked since 2022-09-11.

Hi @Newbie012 👋 First of all, thanks again for this library, so amazing!

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

Reading the first Before we begin section on the Postgres.js docs page, it becomes clear that idiomatic Postgres.js is not supported:

SafeQL supports by design only the following pattern: ...

Screen Shot 2022-09-11 at 17 39 15

It would be nice to be able to keep the code simple when using SafeQL and not have to create an extra wrapper for every project using Postgres.js + SafeQL.

For us, we would like to avoid any extra code / boilerplate because we are teaching beginners to programming.

Describe the solution you'd like

It would be great to remove this limitation and allow for idiomatic Postgres.js as in their docs:

// db.js
import postgres from 'postgres'

const sql = postgres({ /* options */ }) // will use psql environment variables

export default sql

// users.js
import sql from './db.js'

async function getUsersOver(age) {
  const users = await sql`
    select
      name,
      age
    from users
    where age > ${ age }
  `
  // users = Result [{ name: "Walter", age: 80 }, { name: 'Murray', age: 68 }, ...]
  return users
}

If the "by design" limitation is about the ESLint AST, I guess there are a few different ways this could be approached, so that it can support await sql()...

Describe alternatives you've considered

Staying with the wrapper

Additional context

This would also help the plugin with its claim of "easy to use and integrate with your existing codebase":

Screen Shot 2022-09-11 at 17 52 43

Newbie012 wrote this answer on 2022-09-11

Thanks for the feedback! It means much to me.

This is doable on my side, but I have one problem:

  // Type: postgres.PendingQuery<postgres.Row[]>
  const a = sql`SELECT id FROM starship`;

  // Error: Type '{ id: number; }' does not satisfy the constraint 'readonly (object | undefined)[]'.
  const b = sql<{ id: number }>`SELECT id FROM starship`;
                ~~~~~~~~~~~~~~
  // Type: postgres.PendingQuery<{ id: number; }[]>
  const c = sql<{ id: number }[]>`SELECT id FROM starship`;

The fact that Postgres.js generic has to be an array is problematic since all other popular libraries don't require that (which makes sense since it would be redundant). The fix would be easy, but it would be a breaking change for Postgres.js.

Other libraries:

// pg: QueryResult<{ id: number; }>
const q = await client.query<{ id: number }>("SELECT id FROM starship")
q.rows[0]; // { id: number }

// sequelize: { id: number; }[]
sequelize.query<{ id: number }>("SELECT id FROM starship", { type: QueryTypes.SELECT });

According to TypeORM's query method, it doesn't accept a generic type:

query(query: string, parameters?: any[]): Promise<any>;
karlhorky wrote this answer on 2022-09-12

Oh interesting, I wasn't aware of that about other libraries.

I assume this is a problem because of how the generic parameters are being used in SafeQL? Could SafeQL support both using nested conditional types?

Newbie012 wrote this answer on 2022-09-12

I'm not too fond of this approach since it feels a bit hacky.

Another option, is to create a new property called transformType:

// Will be used in .eslintrc.json since functions are not available in JSON
type TransformTypeString = `\${type}${string}` | `${string}\${type}`;

// Will be used in .eslintrc.js since it gives more power
type TransformTypeFn = (type: string) => string;

type TransformType = TransformTypeString | TransformTypeFn;

const a: TransformType = (type) => type;
const b: TransformType = "${type}";

const aa: TransformType = (type) => `${type}[]`;
const bb: TransformType = "${type}[]";

This approach may solve future issues as well.

It should look like:

// .eslintrc.json
{
  // ...
  "rules": {
    // ...
    "@ts-safeql/check-sql": [
      "error",
      {
        "connections": [
          {
            "migrationsDir": "./your_migrations_folder_path",
            "databaseName": "my_db_shadow",
            "name": "sql",
            "transformType": "${type}[]"
          }
        ]
      }
    ]
  }
}
karlhorky wrote this answer on 2022-09-13

Interesting, yeah making it configurable was another option I was considering - I didn't write about it because I thought it would be a bit overkill for a single extra type. But I understand if the internals are more complex than I imagined.

So am I understanding correctly that a user would just configure the ESLint plugin using "transformType": "${type}[]", and then be able to use Postgres.js as usual?


I suppose an alternative configuration (pros: less error prone and less implementation effort, potential cons: less flexible) would be something like: "driver": "postgres", similar to what ley does: https://github.com/lukeed/ley#optsdriver

Oh and the default for driver (also like ley) could be the driver installed in package.json - if there is one. If not, or if multiple drivers are found, then default to the "more common" type that you were describing.

Newbie012 wrote this answer on 2022-09-13

Thanks for the insights :)
My problem with this approach is not about internal complexity. There are none (currently).

The reason ley asks you for a driver is that:

  1. There are very few SQL databases
  2. It has to do more than one thing for each "driver".

In our case, There are dozens of SQL libraries. Writing workarounds for each SQL library will lead to overcomplexity each time we want to introduce new features.

I will eventually support most of the SQL libraries (Postgres.js without a wrapper specifically), but as of writing this comment, I don't see myself explicitly writing "adapters" for each library.

karlhorky wrote this answer on 2022-09-13

Ah, I didn't mean to propose supporting all libraries right away - just driver: 'postgres' and unset / unconfigured would work for now. And reading the postgres package name inside dependencies inside package.json could also auto-configure driver: 'postgres', so that it would be zero-config for a lot of simple use cases.

Newbie012 wrote this answer on 2022-09-14
karlhorky wrote this answer on 2022-09-14

Nice, thanks for this!! Quick PR for the typo: #24

karlhorky wrote this answer on 2022-09-14

Oh, on an unrelated note - would you consider using .eslintrc.js in your examples (instead of .eslintrc.json)?

I noticed that you have JS comments in your examples, which is not really JSON anymore. So copy + paste of those examples could cause problems, if you're suggesting using a JSON file (maybe not a problem in ESLint itself, but a problem for other tooling eg. your IDE).

If you'd be open to this, I can make this change.


As an example, as a developer pressed for time, I would probably navigate to the Postgres.js docs page and copy and paste the following block, which would cause problems in various tooling such as my editor:

Screen Shot 2022-09-14 at 16 22 17

Newbie012 wrote this answer on 2022-09-14

I prefer keeping the current state because:

  1. While the JSON spec doesn't support comments, they are valid in configuration files such as tsconfig.json (which comes with comments by default), and VSCode supports that.
  2. I like to keep things simple. Once I rename it from .json to .js, I lose the autocompletion from the IDE. If there were an option for eslintrc.ts, I would've been ok with it.
  3. I don't think that the current documentation is well written. There are many improvements and tweaks to do for the plugin before it reaches a stable version. Thus, I wouldn't recommend fine-tuning the documentation for now.
karlhorky wrote this answer on 2022-09-14

2. Once I rename it from .json to .js, I lose the autocompletion from the IDE

What about importing the type using JSDoc?

  1. VSCode supports that.

Indeed it does, wasn't aware of that! Then probably not such a big deal.

Screen Shot 2022-09-14 at 17 04 19

More Details About Repo
Owner Name ts-safeql
Repo Name safeql
Full Name ts-safeql/safeql
Language TypeScript
Created Date 2022-09-08
Updated Date 2023-03-16
Star Count 795
Watcher Count 5
Fork Count 14
Issue Count 7

YOU MAY BE INTERESTED

Issue Title Created Date Updated Date