xoid icon indicating copy to clipboard operation
xoid copied to clipboard

Do we need a separate ReadOnly/Computed Atom type?

Open is-jonreeves opened this issue 1 year ago • 1 comments

Overview

It's odd to me that (computed) Atoms like atom(read => read($count) * 2) still have .set and .update methods.

As a "consumer" of the above Atom (without knowledge of its underlying initialization), I'm lead to believe I can mutate it and it won't be an issue. It works fine and subscribers are notified, but state becomes out of sync. If/when the underlying $count is mutated then the prior change is lost but the state is back in sync.

Here is a simplified example:

const createCounter = (initial = 0) => {
  const $count = atom(initial);
  const $double = atom(read => read($count) * 2);

  const increment = () => $count.update(s => s + 1);
  const decrement = () => $count.update(s => s - 1);
  const reset = () => $count.set(initial);

  return {
    $count,
    $double,
    increment,
    decrement,
    reset,
  };
};


const counter = createCounter(0);
counter.$count.subscribe(next => console.log('[Count] Changed:', next));
counter.$double.subscribe(next => console.log('[Double] Changed:', next));

counter.increment();
// [Count] Changed: 1
// [Double] Changed: 2 
// -- { count: 1, double: 2 } ✅

counter.$double.set(10);
// [Double] Changed: 10
// -- { count: 2, double: 10 } ❌-- state out of sync

counter.increment();
// [Count] Changed: 2
// [Double] Changed: 4
// -- { count: 2, double: 4 } ❌ -- back in sync, but can be unexpected

Additional Context

I'm a big fan of xoid and its API, and have been using it on and off for a while now. Mainly just for basic/small state machines in side projects, so I hadn't really butted up against this issue until now.

Recently I needed to build a bunch of separate objects with inter-dependent states where 80% of the props were computed. Traditionally this is really easy with Classes and Getters, but using those objects in a reactive (and performant) way in React/Vue becomes a real pain. I decided to give it a shot with xoid and it was honestly a real joy.

Personally I really liked the approach of using a factory to construct a single Computed Atom with all my mutation actions on it (directly on the value, or optionally via the .actions prop, using the second argument). This allowed me to subscribe to the main object itself or focus on any nested children, and also have control over what can be mutated (via only the actions).

const createCounter = (initial = 0) => {
  // Internal State
  const $count = atom(initial);
  const $double = atom(read => read($count) * 2);

  // Instance
  return atom(read => ({
    count: read($count),
    double: read($double),
    increment: () => $count.update(s => s + 1),
    decrement: () => $count.update(s => s - 1),
    reset: () => $count.set(initial),
  }));
}

const $counter = createCounter(0);
$counter.subscribe(next => console.log(`[*] Changed, ${JSON.stringify(next)}`));
$counter.focus('count').subscribe(next => console.log('[Count] Changed:', next));
$counter.focus('double').subscribe(next => console.log('[Double] Changed:', next));

// NOTE: that in this example trying to mutate either `count` or `double` would cause issues

Everything went well, but when making use of the Atoms I noticed I had access to those .set and .update methods so decided to try them out to see what would happen. That's when I noticed this behavior.

Another potential pitfall here (mainly with my favored approach), is that it can seem like you can mix and match mutable state and computed state on the same Atom, but if you do, you will lose any manually mutated changes. e.g..

const createPerson = (options: { firstName: string, lastName: string }) => {
  const $firstName = atom(options.firstName);
  const $lastName = atom(options.lastName);
  const $fullName = atom(read => `${read($firstName)} ${read($lastName)}`);

  return atom(read => ({
    title: '', // <--- ⛔ Not obviously problematic
    firstName: read($firstName),
    lastName: read($lastName),
    fullName: read($fullName),
    setFirstName: (value: string) => $firstName.set(value),
    setLastName: (value: string) => $lastName.set(value),
  }));
};

const $person = createPerson({ firstName: 'Adam', lastName: 'Ant' });
$person.focus('title').set('Mr.'); // <--- ⛔ Succeeds
$person.value.setFirstName('Beth'); // <--- ⛔ Title changes lost

console.log('Title: ', $person.value.title);

While I don't think there is anything that can be done to prevent someone from creating that title: '' property, if the Computed Atom wasn't mutable it would be quickly obvious this prop was actually readonly.

I guess another approach would be to avoid returning the single Computed Atom and instead implement something like your finite state machines example. But, I'm not entirely sure how you'd make use of computed values?

Suggestion

I don't know if there is a simple way to maybe remove .set and .update from computed Atoms, but I think it might be a good idea if it doesn't really serve any purpose or might mislead.

Additionally, I'm wondering if atom() is trying to do too many things with its ability to take a function. I know nanostores has a separate computed() function as does both Vue and Svelte. Maybe there is a good reason for this separation?

is-jonreeves avatar May 13 '24 22:05 is-jonreeves

Hi @is-jonreeves , thank you for sharing your views, and your kind words about xoid. 🙂 I think you've made points here that are difficult to refute. Also thank you for taking the time to explain them in a very clear manner.

I'll start with this one:

Additionally, I'm wondering if atom() is trying to do too many things with its ability to take a function.

This is something I think about once in a while, especially for "better tree-shaking" purposes. I cannot say I disagree with this. This was an early decision I have made, because it was attractive to say "xoid has only 1 export!" (Before inject and effect exports were added). I think it helped making the first introduction to the library exciting for learning, and it was a nice selling point. Also Jotai as a popular library was doing the same thing.

To discuss this better, I need to inform you about my vision for xoid in the close future: I'll add the following exports to the main package. (which are under @xoid/reactive package at the moment) After this, xoid will be comparable to a subset of @vue/reactivity, but it'll keep focusing on small size and API.

Screenshot 2024-06-14 at 13 39 04

So, xoid's exports are currently 3 (the center column), but there will be 6 more exports soon (right column). (The leftmost column will not be exported from the root. They'll only be in xoid/core route for advanced usage.) With this in mind, I think there are these disadvantages to create a separate export, let's say "derived", for the ability to take a function:

  • Naming the new export as derived, would be inaccurate because the function form is sometimes used just for lazy evaluation: const $my = atom(() => expensiveComputation()), ie, it's not always derived state.
  • derived will introduce an extra export which we try to avoid if possible
  • Adding an extra derived function also implies a Derived type similar to Atom. I also try to introducing new types if possible.
  • Developer experience concerns:
    • There will be a computed export (for implicit dependency collection), a new export with a similar name can be confusing.
    • It's easier to think of atom as the "powerhouse" that aggregates all the "core" functionalities. (This one might be too personal, but with all the other functions, and especially the "computed", I want our nomenclature to shrink for cognitive reasons)
    • Last but not least, all these being said, it doesn't seem strong enough to make a breaking change

Also maybe this is off-topic, but I think atoms having .focus and .map methods can also be considered as trying to do many things (Sometimes I think this way too). To me, their existence is justified for the following reason. When you have them as methods, you can nicely chain them:

const $derivedAtom = $atom
  .focus('event')
  .map((e) => ({ x: e.clientX, y: e.clientY }))
  .map(calculateOffset)

If they were exports instead, the sam would look like:

import { map, focus } from 'xoid'

const $derivedAtom = map(
  map(
    focus($atom, 'event'), 
    (e) => ({ x: e.clientX, y: e.clientY })
  ),
  calculateOffset
)

I hope this was a satisfying response for one of your concerns.

Your case for .set and .update in derived atoms is much more complex. I definitely understand the concerns there. It's not immediately obvious what's wrong with the second example, it requires thinking for a while. I'll try to reason/experiment about it more, but my immediate thinking is that since I can come up with scenarios where the developer should be able to update a derived atom (ie. writable derived atoms should exist), it doesn't make sense to ban writability right away when creating derived atoms. I thought maybe the answer to your concern is introducing a readonly export, just like Vue does, but this seems deeper than that. I need to think about this more.

Again, thank you so much for your contribution! 🙂

onurkerimov avatar Jun 14 '24 11:06 onurkerimov