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

Types: Add types for ReadonlyObservable, ReaonlyObservableArray, ReadonlyComputed #2476

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Retsam
Copy link
Contributor

@Retsam Retsam commented Apr 12, 2019

This adds types for representing immutable versions of observables, observable arrays, and computeds. The changes in this PR match changes that were introduced into the public typings in DefinitelyTyped/DefinitelyTyped#30499.

Although the underlying objects may actually be mutable, it's frequently useful to treat them as if they weren't: functions can accept readonly versions to indicate that they won't modify the contents, or things can be made publicly immutable, while still being privately mutable.

class Counter {
    value: ReadonlyObservable<number>;
    incr: () => void;

    constructor() {
        const value = this.value = ko.observable(0);
        this.incr = () => value(value.peek() + 1);
    }
}

The consumer can read the value, subscribe to it, or call incr to increment it, but they can't set arbitrary values without circumventing the types.


Additionally, while Computed<T> is no longer assignable to Observable<T> like it was in the public typings (missing the '.willMutate' methods and the '.valueHasMutatedand.valueWillMutatemethods), a computed is assignable toReadonlyObservable`, which is convenient for functions like:

function logChanges(observable: ReadonlyObservable<any>);

instead of:

function logChanges(observable: KnockoutObservable<any> | KnockoutComputed<any>);

In theory, certain overrides of ko.computed should return readonly computeds, not writable computeds:

const five = ko.computed(() => 5)
five(6); // Runtime error but not type error

This PR does not address that issue. It never was addressed in the public typings - it caused a prohibitive number of type errors in our project, but it might be a smoother transition here. (A lot of the 'prohibitive errors' are type errors we're going to need to fix to go from 3.4 to 3.5 anyway)

Observables can be cast to readonly observable to indicate that they shouldn't be mutated, or functions can accept readonly observables to indicate that they won't mutate them
ObservableArrays can be cast to this type to prevent mutation, or taken as function arguments to indicate that the function will not mutate the observable
Computeds can be cast to this type to prevent mutation, or taken as function arguments to indicate that the function will not write to this computed

In theory certain ko.computed overrides should return this, but this is not currently the case
read(); // $ExpectType string
read.subscribe(() => {}); // Can still subscribe
// But can't write to it
// read("foo") // $ExpectError // Don't currently have a good mechanism for testing this outside of DT repo
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is pretty much copied from the DefinitelyTyped repo; unfortunately it's pretty hard to write a test that ensures a particular expression doesn't compile in TS. They have some fairly sophisticated test mechanisms that allowed invalid lines to be annotated with this //$ExpectError comment, and the test would fail if the line didn't error.

I couldn't find a good way to replicate that here, generally, so I just left the comments for posterity.

@mbest
Copy link
Member

mbest commented Feb 28, 2021

@Retsam I've made some changes in #2563. Let me know what you think.

@mbest mbest added the waiting label Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants