-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add support for optimistic writes #14
base: main
Are you sure you want to change the base?
Conversation
lib/phoenix/sync/write.ex
Outdated
Provides [optimistic write](https://electric-sql.com/docs/guides/writes) | ||
support for Phoenix- or Plug-based apps. | ||
|
||
The client uses some local database, e.g. [PGLite](https://pglite.dev/), and |
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.
tanstack/optimistic has its own store — it wouldn't work per se directly w/ pglite.
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.
Super cool to see this. I've left a few comments about the documentation. I'm happy to do a pass on it if useful but perhaps we can discuss first.
lib/phoenix/sync/write.ex
Outdated
The client uses some local database and syncs changes into this database from | ||
the server using [Electric](https://electric-sql.com/). | ||
|
||
Local writes are performed on the local database using |
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.
I think it's important decouple the write handling in Phoenix from the use of TanStack/optimistic. Instead, what we can do is define the data structure that this handles. It's then possible for any front-end system that's building up a set of local changes to send this structure over the wire. TanStack/optimistic is one implementation of a client side library that does this.
Ideally we want to extract the structure, call it something and document it standalone. With reference validator, like a JSON Schema or whatever.
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.
Wasn't sure about the exact relation between this and tanstack/optimistic. Essentially it is decoupled, but I think it's helpful to have some reference impl that makes it easy to try this out.
The Phoenix.Sync.Write.Mutation
module has some docs around the data structure that I can expand and link to.
lib/phoenix/sync/write.ex
Outdated
Provides [optimistic write](https://electric-sql.com/docs/guides/writes) | ||
support for Phoenix- or Plug-based apps. | ||
|
||
The client uses some local database and syncs changes into this database from |
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.
This isn't really relevant or true.
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.
oops
lib/phoenix/sync/write.ex
Outdated
@@ -0,0 +1,970 @@ | |||
defmodule Phoenix.Sync.Write do |
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.
Naming wise, I would lean to Writes
over Write
. However, you call the controller MutationsController
, so perhaps this is Phoenix.Sync.Mutations
?
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.
Mutations
was what I was leaning to
lib/phoenix/sync/write.ex
Outdated
|
||
## Client Code | ||
|
||
See [TanStack/optimistic](https://github.com/TanStack/optimistic) for more information. |
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.
This needs to be presented as one example approach. I'm also not sure it belongs here in the moduledoc?
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.
Maybe not the full integration code -- likely to just fall out of date anyway. Do think it's worth pointing to front-end libs here though. Maybe just a list
lib/phoenix/sync/write.ex
Outdated
end | ||
``` | ||
|
||
- `allow/1` creates a write configuration that allows writes from the `todos` |
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.
I don't really understand what's going on here. This allows function seems to be allowing all changes to the table. Is this some new authorization DSL? How does it integrate with per-transaction or per-mutation validation and authorization logic?
I see below:
See
allow/3
for more information on the configuration options for each table.
Is that where more custom logic can be wired in?
Generally I think we have to be very careful about how and what we're introducing here and we must put it in context with the explanation first.
lib/phoenix/sync/write.ex
Outdated
But it's vitally important to remember that the `data` and | ||
`changes` are **untrusted** values. | ||
|
||
Unlike standard uses of `Ecto.Changeset` functions, where |
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.
Could we separate out the auth layer in this stack then? I.e.: have a auth
callback and a changes
callback, so that the data can be authorized before passing to the standard changes function?
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.
Rather than have two functions, maybe just rename changeset
to something else, like validate
or something. Within that we can call the standard changeset/2
function and then do the auth[n,z] of the resulting changeset.
lib/phoenix/sync/write.ex
Outdated
[TanStack/optimistic](https://github.com/TanStack/optimistic) which POSTs | ||
transactions to the Elixir/Phoenix application. | ||
|
||
A transaction is a list of "mutations", a series of `INSERT`, `UPDATE` and |
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.
Generally, I think we need to expand on the rationale for this module better. The point of this integration is to ingest these transactions in a way that leverages your existing auth and context functions. I think we need to say this more clearly and we need to set out an example up front that shows this.
I.e.: say you have an existing Auth plug and an existing changes function, this is how you wire this in to re-use to auth and validate changes coming in through this endpoint. I think showing this re-use of existing code to ingest transactions is the key thing to communicate around the rationale for this code.
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.
There's enough of a difference between controller-based auth and this mutation-based version that it will always require some kind of mapping/refactoring to make the validations usable from both contexts. Not sure how much of that is up to us to explain and how much is just about the dev understanding the domain.
But yes, I've focused on the API docs, not the rationale
remove comments
move docs for plug_opts to top-level
so we can get custom errors
Specifically supporting https://github.com/TanStack/optimistic