statelyai / rfcs

RFCs for XState and Stately tools
44 stars 5 forks source link

Computed context #13

Closed davidkpiano closed 2 years ago

davidkpiano commented 2 years ago

View rendered text

mellson commented 2 years ago

I think this would be a really great addition to XState. And since this is optional, I don't believe the drawback of making the API surface bigger should hold this back.

zachallaun commented 2 years ago

I can certainly see how this would be convenient in many cases! However, I thought it would be useful to provide a quick-and-dirty alternative using features the language already gives us:

class Updatable {
  update(changes) {
    return Object.assign(
      Object.create(Object.getPrototypeOf(this)),
      changes
    )
  }
}

class Person extends Updatable {
  firstName = ''
  lastName = ''
  age

  get fullName() {
    return `${this.firstName} ${this.lastName}`
  }
}

const machine = createMachine({
  context: {
    person: new Person(),
  },
  entry: assign({
    person: (_, event) => person.update({ firstName: event.value }),
  }),
  exit: [
    assign({
      person: (_, event) => person.update({ lastName: event.value }),
    }),
    assign({
      person: (context, event) => {
        // As with `computed` example, the assign above will have
        // already taken place
        console.log(context.person.fullName)
        return person.update({ age: event.value })
      }
    })
  ],
  // ...
})

To continue playing devil's advocate a bit: A domain object requiring computed properties may be an indication that it should be extracted into something well-defined, as opposed to being defined in context. While the above is more code overall than using the proposed computed field, the machine definition itself is arguably simpler.

This counterexample, however, has the notable drawback that person is no longer easily serializable and goes against the recommendation that context contain only plain objects. I wonder, then, whether it might make more sense to support custom (de)serialization hooks to allow more complete usage of language features?

I'm certainly not opposed to the RFC, but thought it would be worth sharing the above in case it sparks any useful conversation.

davidkpiano commented 2 years ago

I wonder, then, whether it might make more sense to support custom (de)serialization hooks to allow more complete usage of language features?

That could be a good idea; want to open a separate RFC or a feature request for this?

But yes, in general, the downsides are that custom computed values are no longer a first-class citizen, and there now become n number of ways to do it, which makes it non-visualizable.

Andarist commented 2 years ago

I very much sympathize with one of the mentioned drawbacks in the RFC:

"I'd personally want a stronger reason than "convenience" for expanding the API (ie. adding to the amount of stuff I need to learn)" (Discord)

Especially that, with the current state of this RFC, comparing this to MobX/Pinia/Vue is like comparing apples and oranges. The RFC focuses on the syntax~/API but only briefly mentions that it's missing the crucial functionality piece (at least the functionality piece that is in all of those mentioned libraries).

A big part of those other APIs is that they are included in observable libraries and thus they come with fine-grained dependency tracking and they build caching on top of that. We are not an observable library and thus we can't easily add a proper caching layer here - or rather, we'd have to include new internal mechanism to enable caching. That is different from all of those mentioned libraries because they build caching on top of what they already have, they build on top of existing principles/traits of their system.

The RFC mentions that:

Caching optimizations may be possible in the future

And I agree with the statement - but I think that if anything this should not be a future thing of this feature. If we decide to move forward with this then caching should be included right from the start. The problem I see though is that this won't be trivial and would bring some additional complexity to the codebase and ultimately I'm not sure if it's worth it nor if it's our place to even include such high-level APIs in the first place.

There is definitely a good chunk of our userbase that would enjoy this kind of a feature but there is also a good chunk of our userbase that would probably not use this feature. It seems like it all boils down to familiarity with some of the other frameworks - Vue/MobX/etc devs would like to use this whereas for React/Redux devs this would be a step outside of their comfort zone. In the context of popular backend solutions, this pattern is probably also not among the top-request features. It seems to me that this is mainly familiar to users of those observable frontend libraries.

If you feel strongly about this feature - have you considered implementing it as a plugin? Such a solution would add some complexity to the codebase (we don't have any plugins right now) but it would allow isolating this from the core. The question here is - do we envision adding other plugins in the future? Another alternative would be to implement this using a decorator approach withComputed(createMachine({}), { computed: {} }) (or smth like that) if we'd juggle some internal APIs differently to allow hooking into them from the outside~.

It also seems to me that mixing computed values into context is bad - both are defined separately but then suddenly used in a similar way (by just accessing a context property). It also raises questions as to how are you supposed to type this - this subject is not touched by the RFC at all. I think that including the PoC of types in the RFC might often showcase the complexity of the feature and the chosen design to the author - perhaps even lead them to other, better, designs. Usually what is easier to type is also easier to understand by the users.

An important argument for making this a first-class citizen has been raised - a first-class thing is visualizable. And I totally agree with the premise of this statement. I think though that it should be mentioned why this is important to be visualized, how it would be visualized and how helpful that might be in practice. Imagining possible visualizations has been left out to the reader here, many won't think it through and might just accept it as-is without rethinking the implications. Throwing "visualizable" as an argument without backing it with examples might feel a little bit like a trump card has been thrown, beating out any other counter-arguments, but in practice, we should focus on the actual applications of a given visualization possibility.

davidkpiano commented 2 years ago

This might be better as part of a helper library, as a wrapper on top of machine rather than within the machine itself.