farmOS / field-kit

A modular, offline-first companion app to farmOS.
https://farmOS.org
GNU General Public License v3.0
59 stars 38 forks source link

Adopt `checkout`/`revise`/`commit` API, instead of `load`/`fetch`/`sync`, for working with entities #494

Closed jgaehring closed 2 years ago

jgaehring commented 2 years ago

This draws on some ideas formed, and conclusions arrived at, in two previous issues:

It's probably best summed up in https://github.com/farmOS/field-kit/issues/274#issuecomment-1008008036:

Under the current paradigm, modules can call loadLogs (or use the corresponding load* method for any other entity) to access logs in IDB. They're requested from the database, then loaded into the Vuex store, where they are automatically passed to the modules root components as this.logs. They are then modified via updateLog and those modifications sent to the server with syncLogs.

At least the very least I need to spend a little time to reevaluate that paradigm, and the overall conceptual framework for how farm data is accessed and modified by modules. Yesterday we tossed around the idea of a "draft" state, but this gets a little vague and conflates the primarily "DX-side" concept of an invalid or pending state of data, with the "UX-side" concept of an incomplete task, more or less equivalent to { status: 'in_progress' }.

One idea is to shift away from this "loading/syncing" paradigm towards more of a "checkout" or "read/write" paradigm, where a module requests access to write to an entity and must manually relinquish that access before the changes are synced. There could be some kind of intermediary caching layer in there, too, so changes can't be lost, but are partitioned somehow from what's stored in the regular IDB stores.

Another idea is to use entity fields as a more "atomic" unit of change, so that we don't persist changes that happen while, say, the user is typing a message into the notes field, but only once they "leave" that field (ie, onblur). Again, this could be accompanied by some form of intermediary caching. Rules for this could be enforced if modules defined their own conventions with stricter validation than is normally applied to entities. So, for instance, if there is a module for moving animals between paddocks, it could create a convention on top of a particular type of movement log that requires an animal and location to be selected before it can be considered valid. Then this valid state could be a condition for whether or not the log is synced.

Perhaps some combination of these approaches could work, too, but in any event, these considerations would have significant bearing on the final API we present to Field Modules, so it seems prudent to evaluate them now, before beta release.

jgaehring commented 2 years ago

From https://github.com/farmOS/field-kit/issues/493#issuecomment-1059234978:

That last one, useEntities, is a Vue "composable", analogous to React hooks, and at the moment I'm leaning most heavily towards this approach to scoping the checkout/revise/commit functions (#494), which is what useEntities would return.

(Note: I started writing more in that issue, expanding on the pros and cons of putting it on the global lib namespace, but it seems better to document these ideas here...)

The biggest question is how to expose it. An alternative to the global is to pass them down as provider functions (#485), so I could track which module made a particular call to checkout or commit etc. That could be useful for implementing a "backup" mechanism that can isolate uncommitted revisions to a single module and store them separately, so if/when they are retrieved they don't end up munged together with revisions from other modules.

I'm wondering now, though, if it might just be possible to have a useBackup to create a separate bit of state that can be passed in as an option to either useEntities or checkout or both. So something like this:

const backup = useBackup({ key: 'tasks-edit' });
const options = { backup };
const { checkout, revise, commit } = useEntites(options);

// And maybe even provide that backup instance with a method like this...
const uncommittedChanges = backup.recall('log', type, id);

I kinda like that. I haven't thought about all the implementation details but it seems perfectly doable. And it creates a nice pattern of favoring standalone utilities like that, rather than dumping everything into providers.

jgaehring commented 2 years ago

That could be useful for implementing a "backup" mechanism that can isolate uncommitted revisions to a single module and store them separately, so if/when they are retrieved they don't end up munged together with revisions from other modules. I'm wondering now, though, if it might just be possible to have a useBackup to create a separate bit of state that can be passed in as an option to either useEntities or checkout or both. [...] [I like how] it creates a nice pattern of favoring standalone utilities like that, rather than dumping everything into providers.

I'm thinking even more about this and how it all interrelates to what is provided to modules and how different actions can be attributed to them. I started to propose introducing some kind of identifier to every module, via provide/inject, but I keep forgetting that modules already have a de facto identifier in the form of the current route, which must be tied to a unique module as defined by the routes object included in their module.config.js. For issues like the backup, this could be accessed via useRoute at any time during the execution, and could in fact be even more useful since it will also indicate the precise nested route where the revision was checked out.

jgaehring commented 2 years ago

Just including the docs from Orbit.js, brought to my attention by @mstenta via @symbioquine:

https://orbitjs.com/docs/getting-started

Looks like I might have replicated some of what this library does here, but I'm pretty far along at this point and I don't want to rip it all out just now, since I don't think it'd be a simple plug-and-play solution. That said, I can totally see how it might be worthwhile revisiting this once I have a better idea of the trade-offs, or if we find a more robust implementation is needed.