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

Referentially transparent API #30

Closed
steida opened this issue Dec 7, 2021 · 4 comments
Closed

Referentially transparent API #30

steida opened this issue Dec 7, 2021 · 4 comments
Labels
api Related to library's API enhancement New feature or request wontfix This will not be worked on

Comments

@steida
Copy link

steida commented Dec 7, 2021

An expression is called referentially transparent if it can be replaced with its corresponding value (and vice-versa) without changing the program's behavior.

It would simplify Kysely usage and unlock further optimizations. And it's little change I believe.
Take a look how I'm using Kysely in React:

const { data, refresh } = useThingsDB(
  useCallback((db) => db.selectFrom('thing').selectAll().execute(), []),
);

useThingsDB is React Hook (composed from other hooks but that's irrelevant now), which takes Kysely query as a parameter. Note useCallback usage. It makes callback stable because it must be stable.

What am I proposing is toString overload that would serialize Kysely toString and fromString that would deserialize it. With such API, it would be easy to cache etc. Kysely queries.

const { data, refresh } = useThingsDB((db) =>
  db.selectFrom('thing').selectAll(),
);

What do you think?

@koskimas
Copy link
Member

koskimas commented Dec 7, 2021

So you want to be able to serialize queries, not the whole Kysely instance with drivers, connections, plugins and all?

So something like this:

const serialized: string = db.selectFrom('thing').selectAll().serialize()
const query = db.deserialize(serialized)

That should be doable, but the type of db.deserialize(serialized) would obviously be wrong, since we can't deserialize the type from just a string. The deserialized query would be something like QueryBuilder<DB, any, unknown>.

I don't think we should override toString(). People would assume toString() returns the SQL and fromString parses SQL into a QueryBuilder.

You can already serialize the query like this:

const serialized = JSON.stringify(query.toOperationNode())

@steida
Copy link
Author

steida commented Dec 7, 2021

Yes, only the query.

Agreed, toString is too invasive. JSON is better, yes.

toOperation node is probably good enough, maybe we don't need deserialization at all. Will check it.

Thank you for the answer.

@steida
Copy link
Author

steida commented Dec 7, 2021

@koskimas Is there an interface representing a valid SQL? To detect:

db.selectFrom('thing') not valid
db.selectFrom('thing').selectAll() valid, toOperationNode can be called.

@steida
Copy link
Author

steida commented Dec 7, 2021

Hmm, forget about it. It's specialized case. I don't want to distract you.

@steida steida closed this as completed Dec 7, 2021
@igalklebanov igalklebanov added enhancement New feature or request api Related to library's API wontfix This will not be worked on labels Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to library's API enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants