Refactor Signal Class Hierarchy for Improved Type Safety and Extensibility

[DeepDoge]

Currently, State and Computed are defined as standalone classes within the Signal namespace:

interface Signal<T> {}
namespace Signal {
  class State<T> {}
  class Computed<T> {}
}

I suggest a refactor where State and Computed extend a base Signal class:

class Signal<T> {}
namespace Signal {
  class State<T> extends Signal<T> {}
  class Computed<T> extends Signal<T> {}
}

With this structure, we can perform type checks using a single base class:

signalOrValue instanceof Signal;

This is more straightforward and efficient than building a custom type guard function like:

function isSignal(value: unknown): value is Signal

The base Signal class can be read-only, while State, extending Signal, can expose the set() method, and Computed can set() the signal internally. So when I say signalOrValue instanceof Signal I don't care about if it's a State or Computed, i just know its something that can change and I can watch those changes.

Of course I might be missing something here, in that case I would like to know why?

[NullVoxPopuli]

I can't speak for everyone here, but I'm a big fan of this proposed type change. 🎉

I think we have some subclassing tests in the polyfill (but if we don't, we should add them).

Could be worth a PR to try it out and see what happens and then post the results back here? (for me, it's usually easier to discuss things with concrete code -- cause everything is too complicated)

https://github.com/proposal-signals/signal-polyfill

[shaylew]

I think the question is: what happens if you subclass Signal directly? Does its constructor simply throw, except when constructing a Computed or State?

I'm trying to remember what other precedents there are in the standard. The base TypedArray doesn't have a global name (though you can still get it as the prototype of the concrete typed array classes); are there cases where an abstract base class is exposed?

IMO this may be a reason to come up with some more general thing that's interoperable with signals and which State and Computed are special cases of (with a more convenient API and likely their own tuning in implementations), but absent the capability to subclass or instantiate Signal I'm not as sure about exposing it as a class.

[kcastellino]

If the main use case is for a consumer to check whether an object they're holding is a genuine signal, it's probably better to solve that directly, if for no other reason than to improve forward compatibility - if new classes implementing Signal are added in the future, consumers should be able to check for them using the same built-in function. To do that we could spec an additional static method on the Signal namespace, either:

1. @@hasInstance(), which would mean you could do signalOrValue instanceof Signal to check if you've got a signal. This provides a neat interface for type-checks, but does have the side effect that Signal would then look like an "abstract base class" which doesn't actually exist, which could constrain future development.
2. isSignal(), which would be an explicit method to check whether something is a signal, much like Array.isArray(). This keeps the exposed API surface smaller, but introduces another "inconsistency" when type-checking.

Or you could do both, why not? Either way, a pretty basic implementation would look a lot like this (as expressed in TypeScript):

namespace Signal {
  [Symbol.hasInstance](value: any): value is Signal<T> {
    return value instanceof Signal.State || value instanceof Signal.Computed;
  }
}

Although that's not quite how I would spec it, and most engine implementers would probably do something else entirely.

[DeepDoge]

@kcastellino This would work, but there is one problem on the type side. Typescript doesn't support symbol properties on namespaces yet (https://github.com/microsoft/TypeScript/issues/36813).

So .d.ts would look like this instead:

interface Signal<T> {}
declare namespace Signal {
  interface State<T> extends Signal<T> {}
  interface Computed<T> extends Signal<T> {}
}
declare const Signal: {
  [Symbol.hasInstance](value: unknown): value is Signal<unknown>;
  State: { new <T>(): Signal.State<T> };
  Computed: { new <T>(): Signal.Computed<T> };
};

[dead-claudia]

This would work, but there is one problem on the type side. Typescript doesn't support symbol properties on namespaces yet (https://github.com/microsoft/TypeScript/issues/36813).

@DeepDoge

TypeScript can just use a value with the @@hasInstance property, and then use namespace merging to add the member classes. Libraries already often have to use that trick for cases where a function is also a namespace root (it most often occurs in older CommonJS modules, including those that have since been ported to ES modules).

That workaround is probably also why they haven't fixed that issue yet. Otherwise, they wouldn't have been able to type a sizable chunk of decade-old libraries, ranging from Mithril to jQuery to Glob.

[kcastellino]

The more I think about this, the more I think using instanceof checks is the exact wrong way to go about checking whether something is a signal, for the simple reason that the purpose of an instanceof check is to look up the prototype chain. If Signal[Symbol.hasInstance]() checks the prototype, then $foo instanceof Signal will have the same issues as foos instanceof Array - namely, you can fake a signal by overriding the prototype, but the runtime won't treat it as a signal. On the other hand, if Signal[Symbol.hasInstance]() checks for the special "magic" which makes a signal a signal, then $foo instanceof Signal will behave extremely differently to every other instanceof check by not looking up the prototype.

Because of this, I feel that since checking whether an object is a runtime-optimised signal is similar to checking whether an object is a runtime-optimised array, a Signal.isSignal() static method would be consistent with user expectations. Given that Signal.isSignal() has already been proposed in issue #32, discussion of that static method should probably continue over there.

[DeepDoge]

I don't like the idea of Signal[Symbol.hasInstance]() too. I was just trying to please the critics of the idea. But tbh, I see no problems with "faking" a Signal, devs are free to implement their own Signals as they please, its their codebase. Having a Signal class would allow other code to treat custom signals as Signals as well, which is great. People can implement their own signals. And their signals would get treated as Signal without checking for a magic function name. Never been fan of things like Array.isArray(), requires you to complicate your logic in order to check what a thing is.

But JavaScript is filled with weird implementations like for example await keyword treats a PromiseLike as if its a Promise creating never resolving promises, just because some random object has a function named then() in it. We have ArrayLike because we can't break old code. Of course we all would want a composition system, but we are in js, and js doesn't have interfaces. Having a standard is better than not having one.

Basically; I see no difference between a interface Signal and abstract class Signal in this context. It also simplifies the process of creating custom Signals, which will happen. So, it's better custom Signal types are first class citizen from the start.