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

Move non-protocol options like table & where to the params sub-key #2081

Merged
merged 24 commits into from
Dec 3, 2024

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Dec 2, 2024

Fix #2079

Electric's TypeScript client is currently tightly coupled to PostgreSQL-specific options in its ShapeStreamOptions interface. As Electric plans to support multiple data sources in the future, we need to separate protocol-level options from source-specific options.

Changes

  1. Created a new PostgresParams type to define PostgreSQL-specific parameters:
    • table: The root table for the shape
    • where: Where clauses for the shape
    • columns: Columns to include in the shape
    • replica: Whether to send full or partial row updates
  2. Moved PostgreSQL-specific options from the top-level ShapeStreamOptions interface to the params sub-key
  3. Updated ParamsRecord type to include PostgreSQL parameters
  4. Updated the ShapeStream class to handle parameters from the params object
  5. Updated documentation to reflect the changes

Migration Example

Before:

const stream = new ShapeStream({
  url: 'http://localhost:3000/v1/shape',
  table: 'users',
  where: 'id > 100',
  columns: ['id', 'name'],
  replica: 'full'
})

After:

const stream = new ShapeStream({
  url: 'http://localhost:3000/v1/shape',
  params: {
    table: 'users',
    where: 'id > 100',
    columns: ['id', 'name'],
    replica: 'full'
  }
})

Copy link

netlify bot commented Dec 2, 2024

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit a030545
🔍 Latest deploy log https://app.netlify.com/sites/electric-next/deploys/674e15a2c748950008286efb
😎 Deploy Preview https://deploy-preview-2081--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@KyleAMathews
Copy link
Contributor Author

On extending types — we have this issue with Electric Cloud where token and database_id params are required when calling the API.

Copy link
Contributor

github-actions bot commented Dec 2, 2024

Copy link
Contributor

@balegas balegas left a comment

Choose a reason for hiding this comment

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

I always though of what you're calling PostgresParams as the ShapeDefinition. I understand the reasoning that where clauses are specific to SQL/relational databases and not the shape, and that other shapes will have different models, but we also don't know what the API is going to look like two steps down the road.

I also think that except for replica all the parameters belong to the class of SQLParams, where MySQL, etc would also fit. I think this can be addresses with the hierarchy of types and only handled when necessary, but if we're trying to communicate support for multiple databases in the future I think this would be a nice thing to model.

I'm sharing my two cents, but I am happy to accept the changes as they are

@KyleAMathews
Copy link
Contributor Author

The distinction is between the "Shape Definition" and "Shape Log Traversing Definition" (heh) — so yeah PostgresParams defines the shape of data to sync within the context of a Postgres database source. A MySQLParams type would define it for MySQL and so on. There'll definitely be overlap between databases but it seems simpler to keep them separate IMO.

@kevin-dp
Copy link
Contributor

kevin-dp commented Dec 3, 2024

I've not looked into the actual code but i'll share my two-cents about the new params option.
I think that we won't support sources other than Postgres for a long time. Therefore, i don't like polluting the current public API for a potential future feature. Moving all those params into a new params field doesn't really make sense in the current product where we only support syncing from PG. When we introduce syncing from other data sources that will probably be a major release and we could introduce the breaking change at that point.

@KyleAMathews
Copy link
Contributor Author

I'd say this is cleaner now — what's been interesting to me building demo apps is that I basically never use the pg-specific options — because I'm always using an API to proxy shape requests, I just construct URLs generally to get the shapes I need with perhaps a custom parameter or two.

So given proxies, the specific http parameters that the Electric API require already aren't special — they're just one of many possible Shape APIs that will exist.

I also don't think we're very far off from supporting other sources. I prompt-coded one last week that was pretty close.

@KyleAMathews KyleAMathews merged commit e96928e into main Dec 3, 2024
26 of 27 checks passed
@KyleAMathews KyleAMathews deleted the params branch December 3, 2024 16:03
@thruflo
Copy link
Contributor

thruflo commented Dec 4, 2024

I agree that the motivation of supporting non PG sources is a long way off and I would argue we should have an explicit way of defining a shape, via a shape definition.

I think the move to having what is currently the shape definition under a params key is harmless but I’m not sure the motivation behind it holds.

@samwillis
Copy link
Contributor

samwillis commented Dec 4, 2024

My 2 cents... I think this is good. 'params' works at the url params level, and is compatible with any proxy. If we one day add a shape def in json, it's likely a "shape" param and would be under options.params.shape. And maybe a shortcut to it via options.shape.

Maybe we let users add any key+value to params to add to the url?

I do think we need to start thinking what a more expressive definition of a shape is, speculatively, to guide what this api should look like. Include, joins, remapping, even aggregates, are things we may want to do in future. It's worth while before we evolve this api much further.

👍

thruflo added a commit that referenced this pull request Dec 4, 2024
Missed a few places where we need to update the switch to put the shape
definition in the `params` key from
#2081
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.

Move non-protocol options to the params sub-key
5 participants