-
Notifications
You must be signed in to change notification settings - Fork 194
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: data access library #128
Conversation
…d can be used for integration with the DAL.
…e electrify function.
…t tests for null values.
…oved it to top-level client and called them liveUnique, liveMany, liveFirst.
…. Also added support for nested objects in create queries.
Removed own schemas and types and replaced them by schemas and types generated by Prisma + added support for creating objects and nested relations + added support for include argument in create, reads, update, upsert, and delete queries.
Modified queries to provide an 'include' argument only if the user provided such an argument. Fixed implementation of DBDescription.
Modified wa-sqlite driver such that the 'electrify' function is consistent with the other drivers.
Also enforced update on related object in update query to enforce that the object is indeed related and throw an error otherwise (only needed for incoming one-to-many relations).
Renaming the DALNamespace and other classes. Make argument optional for findFirst, liveFirst, findMany, liveMany, deleteMany. Remove absurd-sql driver which unlocks removing the remnants of the proxy machinery and the bridge.
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.
Amazing work. Usage example seem to show that this works quite well, so consider most of these as nitpicks.
One meta-note is that we need to utilize proper docstrings much much more in place of regular comments, especially for exported and public functions, doubly so for any exported or public functions that are part of the API.
I can't say that I read attentively each of 1400 lines in table.ts
, but I followed the more complicated segments and looked at the general codestyle. Is the continuation-style API required because of that one driver that requires continuation-style calls to run multiple statements in the same transaction? Can we discuss the complexity cost of keeping it around?
export class Executor { | ||
constructor(private _adapter: DatabaseAdapter, private _notifier: Notifier) {} | ||
|
||
async runInTransaction( |
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.
codestyle: Function is unused & uncovered by tests, do we need it?
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.
It's indeed unused atm.
It may however be useful at some point if we want to execute some independent statements in a transaction.
Surely the transaction
method could also be used for that but is slightly more verbose.
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.
Amazing work. Usage example seem to show that this works quite well, so consider most of these as nitpicks.
One meta-note is that we need to utilize proper docstrings much much more in place of regular comments, especially for exported and public functions, doubly so for any exported or public functions that are part of the API.
I can't say that I read attentively each of 1400 lines in table.ts
, but I followed the more complicated segments and looked at the general codestyle. Is the continuation-style API required because of that one driver that requires continuation-style calls to run multiple statements in the same transaction? Can we discuss the complexity cost of keeping it around?
Yes, it's because of that. This is a problem of both cordova-sqlite storage (cf. this issue) and react-native-sqlite-storage (cf. this issue). |
…ncoming relation.
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.
Thanks for accommodating my comments
This PR integrates the data access library in the typescript-client.
Fixes VAX-526, VAX-587, VAX-613 and VAX-623
Code structure:
The DAL source files reside in
src/client
.The main functionality is implemented by
src/client/model/table.ts
which represents a table from the DB and provides a Prisma-inspired API for CRUD operations on that table.The entry point of the DAL is the static
ElectricClient.create
method which takes a description of the DB (generated by our custom Prisma generator) as input and generates a namespace that extends the electric namespace with a property for each table.The
src/client/input
folder defines the input types that are accepted by the different methods of the API of a table.The
src/client/execution/executor.ts
file defines anExecutor
class wraps a DB adapter and a notifier and provides methods to execute queries and transactions while also callingpotentiallyChanged
on the notifier.Notes about the code:
The main code is located in
src/client/model/table.ts
.This
Table
class implements the API of the DAL.The queries are executed using one of the supported drivers.
However, drivers have subtle differences: some are synchronous (e.g. better-sqlite3) some are async (e.g. wa-sqlite). Within the async drivers, some support promises, others do not support promises and require continuation passing style using callbacks.
For this reason, the unified adapter API also requires passing callbacks and thus writing code in continuation passing style.
As a result, the
Table
class is also implemented in continuation passing style. Thus, internally, we can compose API calls but have to pass callbacks around.