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: added watch() to Drizzle and Kysely integrations #414

Merged
merged 7 commits into from
Dec 2, 2024

Conversation

Chriztiaan
Copy link
Contributor

@Chriztiaan Chriztiaan commented Nov 19, 2024

This PR improves support for watched queries with Drizzle and Kysely by adding a dedicated watch() implementation to each package.

The original report problem showed a limitation with using Drizzle's toSQL() helper with relational queries which you needed to use Powersync's watch function:

const query = dbWithDrizzle.query.companyTable.findMany({ with: { headquarter: true } })
const resultOnce = await query
console.log(resultOnce) // ✅ this creates an object

powerSyncDb.watch(
  toCompilableQuery(query).compile().sql,
  toCompilableQuery(query).compile().parameters as any[],
  { onResult: (result) => console.log('resultWatched', result.rows?._array) }, // 🤔 this creates an array instead of an object
)

Output:

 Working with direct execute/useQuery [
  {
    "headquarter": {
      "id": "d3415320-dae7-402f-928b-672110800e5c",
      "name": "Bachusstr.",
      "radius": 200
    },
    "id": "7d5a56d3-57cf-43a7-ac2c-4bab54c0631a",
    "name": "Zeitn GmbH"
  }
]
 LOG  resultWatched - broken - array of values [
  {
    "headquarter": "[\"d3415320-dae7-402f-928b-672110800e5c\",\"Bachusstr.\",200,null]",
    "id": "7d5a56d3-57cf-43a7-ac2c-4bab54c0631a",
    "name": "Zeitn GmbH"
  }
]

This is because with some relational queries toSQL() invocations will return a query that contains select json_array for the joined values - which Drizzle would map internally.

The solution is to call execute() on the queries directly instead of mapping the queries with toSQL().

Copy link

changeset-bot bot commented Nov 19, 2024

🦋 Changeset detected

Latest commit: d6d9c10

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@powersync/drizzle-driver Minor
@powersync/kysely-driver Minor
@powersync/common Minor
@powersync/op-sqlite Patch
@powersync/react-native Patch
@powersync/tanstack-react-query Patch
@powersync/web Patch
@powersync/diagnostics-app Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Chriztiaan Chriztiaan marked this pull request as ready for review November 19, 2024 14:04
stevensJourney
stevensJourney previously approved these changes Nov 19, 2024
@rkistner
Copy link
Contributor

We had a similar issue with Kysely and useQuery hooks a while back:
#295

There, the issue was solved by using query.execute() instead of db.getAll(sql).
Since the Drizzle implementation also exposes a similar execute() method, I assume this issue is specific to db.watch(), and it already works correctly with useQuery? Could we do something to let the standard watch methods also accept compiled queries, which will solve the problem for Kysely as well, instead of adding a drizzle-specific watch method?

@Chriztiaan
Copy link
Contributor Author

We had a similar issue with Kysely and useQuery hooks a while back: #295

There, the issue was solved by using query.execute() instead of db.getAll(sql). Since the Drizzle implementation also exposes a similar execute() method, I assume this issue is specific to db.watch(), and it already works correctly with useQuery? Could we do something to let the standard watch methods also accept compiled queries, which will solve the problem for Kysely as well, instead of adding a drizzle-specific watch method?

I agree with this to some extent @rkistner. Solving this problem in a way that also improves watch support in Kysely would be great. However we already overloading the watch() signature which was manageable since the first 2 parameters were always sql and params and this would become messy if we introduced a single query parameter to that mix. Another point is that using powersync.watch() alongside your DrizzleDb or KyselyDb instance is not that ergonomic, it means an additional import across your project instead of just being able to conveniently call drizzleDb.watch() or kyselyDb.watch().


As a possible compromise, could we consider a central watch function implementation (also in common as a util perhaps) that takes in a compilable query. This would then be wrapped for both the Drizzle and Kysely integrations.

If we still prefer to have this watch signature on the AbstractPowerSyncDatabase API, I would prefer if we had it under a different name (but that might make the API become even more confusing).

rkistner
rkistner previously approved these changes Nov 20, 2024
Copy link
Contributor

@rkistner rkistner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense. Happy to go with this approach for now and then potentially re-evaluate later.

@Chriztiaan Chriztiaan dismissed stale reviews from rkistner and stevensJourney via 06a6a74 November 21, 2024 11:56
@Chriztiaan Chriztiaan force-pushed the feature/drizzle-watch branch from 06a6a74 to ff11c5a Compare November 21, 2024 12:20
@Chriztiaan Chriztiaan changed the title feat: added watch() to Drizzle integration feat: added watch() to Drizzle and Kysely integrations Nov 21, 2024
@Chriztiaan
Copy link
Contributor Author

Chriztiaan commented Nov 21, 2024

FYI @stevensJourney @rkistner:
I have added a compilableQueryWatch() to @powersync/common which is wrapped by both Drizzle and Kysely. I also added watch tests to both the ORM packages to show that it works as expected.

@Chriztiaan Chriztiaan merged commit 77a9ed2 into main Dec 2, 2024
6 checks passed
@Chriztiaan Chriztiaan deleted the feature/drizzle-watch branch December 2, 2024 08:42
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.

3 participants