Hi @Newbie012
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":
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>;
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?
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}[]"
}
]
}
]
}
}
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.
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:
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.
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.
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:
I prefer keeping the current state because:
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 |
Issue Title | Created Date | Updated Date |
---|