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(sql.unsafe): add optional typearg for row type #512

Closed
wants to merge 2 commits into from

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Oct 4, 2023

The default type is still a zod type producing any as its output, as before.

This is essentially a non-breaking change that makes sql.unsafe less unsafe at compile time, without changing its runtime behaviour.

Copying the type test to demonstrate how it can be used:

  expectTypeOf(await client.one(sql.unsafe`select 'x' as foo`)).toBeAny();

  expectTypeOf(
    await client.one(sql.unsafe<{ foo: string }>`select 'x' as foo`),
  ).toEqualTypeOf<{ foo: string }>();

Note @gajus: with this change, @slonik/migrator and @slonik/typegen should be able to upgrade and support slonik version 37. It already supports 29 so I guess we could bypass 30, 31, 32, 33, 34, 35, and 36 - i.e. all the breaking changes this library has produced in the last year or so.

The default type is still ZodTypeAny.

This is essentially a non-breaking change that makes sql.unsafe *less* unsafe at compile time, without changing its runtime behaviour.
@mmkal mmkal temporarily deployed to release October 4, 2023 21:29 — with GitHub Actions Inactive
@mmkal mmkal temporarily deployed to release October 4, 2023 21:29 — with GitHub Actions Inactive
@mmkal mmkal temporarily deployed to release October 4, 2023 21:29 — with GitHub Actions Inactive
@mmkal mmkal temporarily deployed to release October 4, 2023 21:29 — with GitHub Actions Inactive
@mmkal mmkal temporarily deployed to release October 4, 2023 21:29 — with GitHub Actions Inactive
@gajus
Copy link
Owner

gajus commented Oct 5, 2023

Thank you @mmkal I will take a look in an afternoon.

Note that I also stated in another thread that I am open to making Slonik non-Zod specific. Just need help with the lift.

I've outlined a rough idea in a new thread #514

@mmkal
Copy link
Contributor Author

mmkal commented Oct 5, 2023

Thanks. Being non-zod specific would be awesome, I could absolutely help with that. I will comment there too.

@mmkal
Copy link
Contributor Author

mmkal commented Nov 6, 2023

@gajus any thoughts on this?

@mmkal
Copy link
Contributor Author

mmkal commented Jan 26, 2024

Ping - @gajus can we merge this?

@gajus
Copy link
Owner

gajus commented Jan 27, 2024

My problem with this is that it encourages the use of unsafe.

The only valid use case for unsafe is prototyping/debugging. I think next version of Slonik will drop unsafe entirely.

I fail to see in what context this would be valuable that's not already covered by what's provided by sql.type.

@slonik/migrator does not need this (?).

Meanwhile, I don't want to promote/encourage usage of @slonik/typegen as I see it as an unsafe pattern. It comes down to my personal belief that an API that bridges two remote systems must provide build time and runtime guarantees in order to be considered reliable.

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.

None yet

2 participants