Support Idiomatic Postgres.js

[karlhorky]

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: ...

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":

[Newbie012]

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]

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]

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]

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]

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]

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]

resolved in 0.0.5.

See updated docs - https://www.safeql.dev/compatibility/postgres.js.html

[karlhorky]

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

[karlhorky]

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:

[Newbie012]

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]

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.