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 (client): extract sync API and make DAL optional #1355

Merged
merged 31 commits into from
Jun 26, 2024

Conversation

kevin-dp
Copy link
Contributor

@kevin-dp kevin-dp commented Jun 13, 2024

This PR extracts the sync method out of the DAL by introducing a subscribe method on the ElectricClient.sync object. To compute the shape requested by the user we need a description of the database schema, namely:

  • Table name, and for each table:
    • columns (column name + PG type)
    • outgoing and incoming relations

The outgoing relations are the FKs in the table that reference a column in (another) table.
The incoming relations are FKs defined in (another) table that reference a column of this table.
The incoming relations are required in order to be able to traverse FKs in the reverse direction, e.g. this is needed when syncing the following shape because there is no FK from issues to comments, only from comments to issues:

{
  "table": "issues",
  "include": {
    "comments": true
  }
}

The database description used to be generated by our generator and included in the generated Electric client. To truly extract the sync API i had to make it independent of the DAL and the generated client. Therefore, i extended the CLI to use the metadata that is provided with the migrations to generate the DB description and bundle that into the app.

Note:

  • I modified our TS satellite client in the e2e tests such that every DAL query has raw equivalent and to dynamically dispatch to the right version depending whether we are running with or without the DAL. I introduced a matrix in CI such that every Satellite e2e test is ran once with the DAL and once without the DAL.

Questions:

  • The modified CLI changes the behavior of electric-sql generate as by default it will no longer generate the DAL. If you want to generate the DAL, you need to run it with the --with-dal flag as in electric-sql generate --with-dal. Is thi acceptable, or we want to keep generating the DAL by default and have a --no-dal flag instead?
  • I modified the format of the table columns in the generated DB description, it was previously a Map<TableName, PgType> and is now represented as a Record<TableName, PgType>. While this is not strictly necessary, this simplifies the generation of the DB description. Downside is that clients generated with the old generator won't work with newer versions of the ts-client and will require re-generating the DAL with the new ts-client. Is this fine or should i change it back to using a Map<TableName, PgType> ?

Copy link

linear bot commented Jun 13, 2024

@kevin-dp kevin-dp force-pushed the kevindp/vax-1931-extract-sync-and-raw branch from cc4b21b to e5e1b5a Compare June 13, 2024 10:30
@kevin-dp kevin-dp marked this pull request as ready for review June 13, 2024 12:34
@kevin-dp kevin-dp requested a review from msfstef June 13, 2024 12:39
@samwillis
Copy link
Contributor

The modified CLI changes the behavior of electric-sql generate as by default it will no longer generate the DAL. If you want to generate the DAL, you need to run it with the --with-dal flag as in electric-sql generate --with-dal. Is thi acceptable, or we want to keep generating the DAL by default and have a --no-dal flag instead?

I would be inclined to support both CLI args and have it default to--with-dal for now, we can then easily merge this PR asap without having to coordinate messaging. It's then a flip of the default at some point in the future.

I modified the format of the table columns in the generated DB description, it was previously a Map<TableName, PgType> and is now represented as a Record<TableName, PgType>. While this is not strictly necessary, this simplifies the generation of the DB description. Downside is that clients generated with the old generator won't work with newer versions of the ts-client and will require re-generating the DAL with the new ts-client. Is this fine or should i change it back to using a Map<TableName, PgType> ?

I much prefer Record<TableName, PgType> 👍

Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

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

Amazing work 🔥 ! Left some comments for clarifications

I agree with @samwillis on keeping the --with-dal to true by default, although we need to decide if we want to show the deprecation warnings already in that case. When we're happy with making a breaking change with a release we just switch the default.

I'm also a fan of using the Record<TableName, PgType>, but it does mean that this update would require a regeneration of the client and we still haven't made it clear if that constitutes a breaking change or not.

clients/typescript/src/cli/migrations/command-generate.ts Outdated Show resolved Hide resolved
clients/typescript/src/cli/util/serialize.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/model/client.ts Show resolved Hide resolved
clients/typescript/src/client/model/client.ts Show resolved Hide resolved
@@ -191,10 +245,12 @@ export class ElectricClient<
return new ElectricClient(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not for this PR but I think we should really move to object arguments rather than positional haha

clients/typescript/src/client/model/sync.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/model/table.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/model/sync.ts Outdated Show resolved Hide resolved
clients/typescript/src/client/model/sync.ts Show resolved Hide resolved
@kevin-dp kevin-dp force-pushed the kevindp/vax-1931-extract-sync-and-raw branch 2 times, most recently from 7a2be9b to 1badd76 Compare June 20, 2024 09:47
@kevin-dp
Copy link
Contributor Author

@samwillis @msfstef I addressed most of your comments. The main change is the flip of the flag default. I kept the --with-dal flag but it defaults to true. If you don't want to generate the DAL, then you pass --with-dal false.

@kevin-dp kevin-dp force-pushed the kevindp/vax-1931-extract-sync-and-raw branch from 742febf to 593915a Compare June 20, 2024 10:13
@kevin-dp kevin-dp requested a review from msfstef June 20, 2024 11:05
Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

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

Looks good, ready to merge! I do think we need to confirm that by merging this, although we are keeping the same current behaviour by default, we will be showing deprecation warnings everywhere.

clients/typescript/src/cli/migrations/command-generate.ts Outdated Show resolved Hide resolved
@kevin-dp kevin-dp force-pushed the kevindp/vax-1931-extract-sync-and-raw branch 2 times, most recently from 7e612d2 to 692a12a Compare June 24, 2024 11:54
@kevin-dp kevin-dp force-pushed the kevindp/vax-1931-extract-sync-and-raw branch from b2f1f0f to f2f3bc7 Compare June 26, 2024 10:50
@kevin-dp kevin-dp merged commit 837ce92 into main Jun 26, 2024
30 checks passed
@kevin-dp kevin-dp deleted the kevindp/vax-1931-extract-sync-and-raw branch June 26, 2024 11:51
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

3 participants