ReadonlySignal should not inherit from Signal type

[KillariDev]

Environment

• I am using @preact/signals

Describe the bug
A clear and concise description of what the bug is.

To Reproduce

const numberSingal: ReadonlySignal<number> = useSignal<number>(0) // ok
const numberSignal: Signal<number> = useComputed<number>(0) // should error

This assignment should not be allowed as signal is not readonly

the bug is here:
https://github.com/preactjs/signals/blob/main/packages/core/src/index.ts#L660

interface ReadonlySignal<T = any> extends Signal<T> {
	readonly value: T;
}

Expected behavior
There should be a type error

[rschristian]

This is the intended behavior I believe.

The purpose of ReadonlySignal is to stop you from writing to an incoming signal, not assert whether the signal can be written to. For example, a component might take a ReadonlySignal to show it only consumes, it won't try to write to that data.

Edit: Besides, the only API to return ReadonlySignal is computed/useComputed, and it makes no sense for a consumer to distinguish between them.

As such, I'll close this on out.

[KillariDev]

This is the intended behavior I believe.

The purpose of ReadonlySignal is to stop you from writing to an incoming signal, not assert whether the signal can be written to. For example, a component might take a ReadonlySignal to show it only consumes, it won't try to write to that data.

Edit: Besides, the only API to return ReadonlySignal is computed/useComputed, and it makes no sense for a consumer to distinguish between them.

As such, I'll close this on out.

I had a bug in my code where I had a component that consumed a ReadonlySignal, but did not edit it. I then computed an ReadonlySignal and passed to it. Everything good. Then I modified the original component to edit the passed on signal and made the component Signal. All was allegedly good in compiling part, but then when I run the code, I got runtime exception which should have been caught at compilation time.

This can be fixed by doing the inheritance the other way around. This is clearly a bug in the library as the library is using typescript wrong.

this is like "Apple" is a string, that should be able to put to things that expect strings. But If a function expects "Apple", passing a string to it is not sufficient, as the string could be "Banana", which clearly is not "Apple", while both of them are strings.

"Apple" is of type string, but string is not type of "Apple"
"Banana" is of type string, but string is not type of "Banana"

[rschristian]

All was allegedly good in compiling part, but then when I run the code, I got runtime exception which should have been caught at compilation time.

I don't see how this could possibly be an issue of the types, passing a Signal as a ReadonlySignal is perfectly valid. You'd need to show a more concrete example, as this is incredibly unclear.

This is clearly a bug in the library as the library is using typescript wrong.

This is very much not the case. It sounds more like a misunderstanding of what this type is meant to guard against. A Signal is a completely valid ReadonlySignal, it's how the API works. ReadonlySignal is just there to raise a warning if you try to set it's .value prop, nothing more.

[KillariDev]

All was allegedly good in compiling part, but then when I run the code, I got runtime exception which should have been caught at compilation time.

I don't see how this could possibly be an issue of the types, passing a Signal as a ReadonlySignal is perfectly valid. You'd need to show a more concrete example, as this is incredibly unclear.

This is clearly a bug in the library as the library is using typescript wrong.

This is very much not the case. It sounds more like a misunderstanding of what this type is meant to guard against. A Signal is a completely valid ReadonlySignal, it's how the API works. ReadonlySignal is just there to raise a warning if you try to set it's .value prop, nothing more.

Hey, Sorry. I had my report the wrong way around (I edited it). Sorry for the confusion. It should be like this:

const numberSingal: ReadonlySignal<number> = useSignal<number>(0) // ok
const numberSignal: Signal<number> = useComputed<number>(0) // should error

import { ReadonlySignal, Signal } from '@preact/signals-core'

declare const readonlySignal: ReadonlySignal<number>
const writeableSignal: Signal<number> = readonlySignal // should error, doesn't
writeableSignal.value = 5 // runtime error!

declare const writeableSignal2: Signal<number>
const readonlySignal2: ReadonlySignal<number> = writeableSignal2 // ok, but only because TypeScript doesn't correctly check readonly (soundness)
readonlySignal2.value = 5 // correctly errors

The problem is that the only difference between the two types is if a property is readonly or not. There is a soundness bug in Typescript. Currently, the readonly modifier prohibits assignments to properties so marked, but it still considers a readonly property to be a match for a mutable property in subtyping and assignability type relationships. A flag is being added to typescript to fix this issue: microsoft/TypeScript#58296 . When this flag is enabled that would break preactjs/signals. This flag is going to be part of strict mode at some point.

[rschristian]

Gotcha, that makes more sense.