tc39 / proposal-signals

A proposal to add signals to JavaScript.
MIT License
2.95k stars 54 forks source link

Signal.State question #124

Open ljharb opened 2 months ago

ljharb commented 2 months ago

From reading Rob's blog post, it seems like in const counter = new Signal.State(0); that counter is a stateful and mutable instance. This means that if i want to give someone the ability to read the state but not mutate it, i have to do counter.get.bind(counter), or () => counter.get(), both of which are inconvenient and easy to forget.

Why not have const { get, set } = Signal.State(0)? iow, have it not be newable, and just vend an object that has two standalone functions on it. Usage would be the same, but it'd be much harder to misuse and much easier to pass capabilities around. (i believe this concept is called "lenses" but i might be wrong)

It'd also avoid adding new classes (and thus inheritance/species questions), which is a bonus to many cohorts.

NullVoxPopuli commented 2 months ago

for ReadOnly, I'd do this -- no need for inheritance (user-land classes are best without inheritance)

class ReadonlyState {
  #state = new Signal.State(0);
  get = () => this.#state.get();
}

let counter = new ReadonlyState();
counter.get(); // 0
let { get } = counter;

get() // 0

Though, I do think it would be worthwhile to freeze State objects, much like we can do with vanilla objects -- this would effectively make them read-only as well. maybe something like:


let state = new Signal.State(0);

state.freeze();

state.set(...) // error! frozen!

Why not have

this can be implemented in userland, and likely would be the get/set destructuring issue would likely not be felt by the average user, as frameworks would provide wrappers around this.

it is nice to have bound methods though! and I do think bound methods should be the default for all classes.

The README says the following:

Both State and Computed Signals are designed to be subclassable, to facilitate frameworks' ability to add additional properties through public and private class fields (as well as methods for using that state).

So, if we're to make a classless wrapper around this API, given its constraints (which I think are fair, and def outside "userland"), we can do this:

function signal(initial) {
  return new Signal.State(initial);
}

This is also the strategy I'm using for my signal-utils library:

And this also aligns with how Starbeam / Glimmer / Ember have been handling value-reactivity primitives (https://tutorial.glimdown.com (though, this low-level primitive is abstracted in something more ergonomic, like a decorator) )

Classes are a useful implementation detail, tho I understand not everyone wants to see them -- these wrappers are terser, so that is a benefit 🥳

ljharb commented 2 months ago

In general, we're not trying to add more subclassable things - the ability to subclass builtins in general is even up for removal, if web compatible.

A wrapper that still exposes an instance wouldn't likely be viable.

Having bound prototype methods wouldn't solve the problem tho, because anyone could Signal.State.prototype.set.call on my instance - and freeze doesn't solve that, because i still want to retain the ability to write to it.

littledan commented 1 month ago

Yes, this capability separation question is an important and subtle design point. We have also been discussing this question in #94. Thanks for raising it.

As you noted, it is easy to wrap the Signal.State API to separate the capabilities; going in the other direction of combining things is also easy. So this is a question of shaping ergonomics to encourage the right default usage pattern, which I agree is to separate capabilities.

However, there are other factors, such as:

The current design is oriented to serve the reactivity core, rather than provide the friendly user interface to developers. We may want to expand scope to try to serve that as well, but IMO doing that well would be a significant expansion with, e.g., reactive objects and arrays [which I would like to eventually include! but probably after we get more experience with the core].

robbiespeed commented 1 month ago

One point in favor of a split get/set API is the freeze method being proposed in #125 becomes unnecessary. Even in terms of possible perf advantage from a frozen signal, if set handler gets GCed then the signal can automatically be frozen/disposed.

robbiespeed commented 1 month ago

Want to add that there is president for readonly constructs like this already in the language. Promise is public readonly, write once from owner, there's now also Promise.withResolvers for easily retrieving the reader (promise) and writer (resolvers).

It would make sense for signals to follow a similar API of public readonly, write many from owner.

ljharb commented 1 month ago

It's not totally unnecessary - without it, to give someone the capability and then deny it would require maintaining your own closed-over variable and wrapping set before vending it.

robbiespeed commented 1 month ago

It's not totally unnecessary - without it, to give someone the capability and then deny it would require maintaining your own closed-over variable and wrapping set before vending it.

Assuming we're talking about freeze. Yes if you wanted to pass ability to write while maintaining the ability to revoke, you'd have to wrap the setter like:

let { get, set: innerSet } = State(1);

const freeze = () => { innerSet = () => {}; };
const set = (v) => innerSet(v);

The benefit here, is that the owner both controls who can freeze or mutate the signal.