Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signal.State question #124

Open
ljharb opened this issue Apr 2, 2024 · 7 comments
Open

Signal.State question #124

ljharb opened this issue Apr 2, 2024 · 7 comments

Comments

@ljharb
Copy link
Member

ljharb commented Apr 2, 2024

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
Copy link
Collaborator

NullVoxPopuli commented Apr 2, 2024

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
Copy link
Member Author

ljharb commented Apr 2, 2024

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
Copy link
Member

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 frameworks that we are aware of who would want a capability-separated design would also want to expose a function-based API, rather than class/method-based. Subclassing is very useful, e.g., to add additional associated fields, but subclasses can't override [[Call]]. So, we'd still expect a wrapper around each "branch"--and if you're doing that wrapper anyway, you might as well have that be the place to implement capability separation.
  • A core design goal here is to ensure that signals are usable in practice, which relates to not doing too many expensive allocations, both of closures and signal objects. Starting with a base API which has a single allocation is cheaper.
  • Semantically, there may be certain operations which are best defined on a getter-setter pair, e.g., Signal class lacks implementation of setting new value using previous value #92.

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
Copy link

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
Copy link

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
Copy link
Member Author

ljharb commented Apr 10, 2024

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
Copy link

robbiespeed commented Apr 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants