-
Notifications
You must be signed in to change notification settings - Fork 149
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): make converter API public #1397
Conversation
b2f1f0f
to
f2f3bc7
Compare
e614240
to
aac4863
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nice work! Left a few minor comments
@@ -1,6 +1,6 @@ | |||
elixir 1.17.0-otp-27 | |||
erlang 27.0 | |||
nodejs 20.2.0 | |||
nodejs 21.4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might need to update the CI workflows to use this version as well in the test matrix, build, etc
@@ -26,3 +64,34 @@ export interface Converter { | |||
export function isDataObject(v: unknown): boolean { | |||
return v instanceof Date || typeof v === 'bigint' || ArrayBuffer.isView(v) | |||
} | |||
|
|||
export function mapRow< | |||
T extends Record<string, unknown> = Record<string, unknown> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might be worth extracting Record<string, unknown>
to a type alias like RowRecord
or something as it's so common
>( | ||
row: Record<string, unknown>, | ||
tableSchema: Pick<AnyTableSchema, 'fields'>, | ||
f: (v: any, pgType: PgType) => any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: more explicit name like convert
? f
feels a little bit too terse haha
>( | ||
rows: Array<Record<string, unknown>>, | ||
tableSchema: Pick<AnyTableSchema, 'fields'>, | ||
f: (v: any, pgType: PgType) => any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same as above
@@ -11,7 +12,7 @@ import { PgBasicType, PgDateType, PgType } from './types' | |||
* Currently, no conversions are needed for the data types we support. | |||
*/ | |||
|
|||
function toPostgres(v: any, pgType: PgType): any { | |||
export function toPostgres(v: any, pgType: PgType): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why are these exported, they are exported via the converter interface no? (same for SQLite)
@@ -221,6 +225,10 @@ export class Table< | |||
* As such, one can compose these methods arbitrarily and then run them inside a single transaction. | |||
*/ | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might need more rebasing? haha
👋 we've been working the last month on a rebuild of the Electric server over at a temporary repo https://github.com/electric-sql/electric-next/ You can read more about why we made the decision at https://next.electric-sql.com/about We're really excited about all the new possibilities the new server brings and we hope you'll check it out soon and give us your feedback. We're now moving the temporary repo back here. As part of that migration we're closing all the old issues and PRs. We really appreciate you taking the time to investigate and create this improvement! |
This PR extends the converter API with
encodeRow(s)
/decodeRow(s)
methods and exports the API.This API is a nice to have when dealing with raw queries and having to correctly serialise/deserialise rows to/from the database.