farmOS / farmOS.js

A JavaScript library for working with farmOS data structures and interacting with farmOS servers.
MIT License
15 stars 13 forks source link

Use immutable API instead of getters and setters #34

Closed jgaehring closed 2 years ago

jgaehring commented 3 years ago

I initially used a getter/setter pattern in the model to provide simpler ergonomics when updating entities without needing to think about metadata. For instance, to set the status on a log:

log.status = 'done';

In the background, this also updates the changed metadata to the current timestamp for the purpose of merging remote changes. This was a change from the earlier farmLog implementation which used an update() function instead:

const log2 = update(log1, { status: 'done' });

Although I thought this would help to simplify update operations, I'm learning in practice that this doesn't make a whole lot of difference, but it would be far more helpful to work with immutable data structures to prevent issues of shared mutable state.

This really wouldn't be too difficult, as it only requires moving the logic out of the getters/setters and returning the updated value, rather than mutating it. Eventually, this could be better for integrating validation into the update function. I believe this could enable better integration with the connect library, too, like performing merges with fetch/send operations, etc.

jgaehring commented 3 years ago

A few common immutable libraries for reference:

jgaehring commented 3 years ago

I feel like this would also eliminate some of the ugliness in creating entities from a remote that hasn't been synced to the local context before. In Field Kit, I have to manually pass the remote metadata in to the create method, which is kinda a pain:

const now = new Date().toISOString();
const { meta } = remote;
const log = farm.log.create(remote, { remote: { lastSync: now, meta } });
jgaehring commented 3 years ago

I have to manually pass the remote metadata in to the create method

It's actually slightly uglier, now that I realize create can't actually handle raw remotes from the server, even with adapter.js in the middle. Instead I have to use deserialize, with a lil help from Ramda:

const now = new Date().toISOString();
const meta = m => ({ remote: { lastSync: now, meta: m } });
const serialized = evolve({ meta }, remote);
const log = farm.log.deserialize(serialized);

If merge could handle all of this, by accepting undefined as a first argument if there's no local entity and returning a brand new local version of the remote, that would be soooo much cleaner. But right now merge acts on the local entity in place, and has no return value. I feel like the whole mental model would become more straightforward with immutability.

jgaehring commented 3 years ago

Instead I have to use deserialize

Another thing: I could eliminate the serialize and deserialize methods.

jgaehring commented 2 years ago

Something I've been debating with this issue is whether to enforce immutability on entities, with something like Object.freeze(). I was curious what other libraries do about this. In the case of automerge, where strict immutability isn't the top concern, they do not enforce it, citing performance costs (automerge/automerge#177). Those are good enough reasons for me, so I'm going to keep such enforcement outside the scope of this issue, though I'm open to exploring it in the future.

jgaehring commented 2 years ago

Resolved by 9019f6f.