SkipLabs / skip

Skip is a framework for building reactive services
https://skiplabs.io
MIT License
156 stars 9 forks source link

ts: add readonly modifier #536

Closed mbouaziz closed 2 days ago

mbouaziz commented 4 days ago

I realized we don't use readonly much, though it's pretty helpful in understanding code.

Like a child who discovers a new word I decided to put it everywhere (it made sense).

Note on style: I turned class properties into parameter properties only when they were all initialized with the same visibility and readonly-ness.

mbouaziz commented 2 days ago

The meaning of readonly is shallow, only saying that a local variable is not assigned to, right?

Yes, only saying that a property is not assigned to after the object is constructed.

In the skipruntime-ts/examples, it might be better to not tag things like EagerCollections as readonly, since I don't think it adds anything, and unless users are already very familiar with the shallow semantics, it might cause confusion by hinting at a read-only collection, when in fact they commonly can be written to from elsewhere.

Right, I'll revert this part.

I wonder, how trustworthy is readonly? Do you have experience failing to break it?

It's only a typing thing and it disappears at runtime. It doesn't bring guarantees outside Typescript. I got warned a few times while writing the diff. Actually I should also add readonly to types and interfaces because otherwise it's too easy to break:

class A {
    constructor (readonly f: number) {}
};
const x = new A(0);
x.f = 1; // type error
const y : { f : number} = x;
y.f = 2; // totally fine 😢
jberdine commented 2 days ago

Actually I should also add readonly to types and interfaces because otherwise it's too easy to break:

You feel that it's all worthwhile? I don't have a strong view either way, but it's not obvious to me.

mbouaziz commented 2 days ago

To avoid any fake readonly guarantees, I kept it only for private fields.

It could still make sense to add more readonly for parameters as a way to document we're not mutating them, but that could be too verbose