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

Class state has a fundamental typing flaw #12655

Open
elliott-with-the-longest-name-on-github opened this issue Jul 29, 2024 · 18 comments
Open

Comments

@elliott-with-the-longest-name-on-github
Copy link
Contributor

Describe the bug

Consider the following example:

export class Foo {
  count: number; // Property 'count' has no initializer and is not definitely assigned in the constructor.(2564)

  constructor() {};
}

In with a Svelte class, there is no way I can find to declare class fields in this way:

export class Foo {
  count: number;

  constructor(initialCount) {
    this.count = $state(initialCount); // `$state(...)` can only be used as a variable declaration initializer or a class field
  }
}

Doesn't work, because $state can't be used in the constructor.

export class Foo {
  count: number = $state(); // typeof count === number | undefined, even though it's _not_ undefined-able. You also lose the constructor assignment analysis

  constructor(initialCount) {
    this.count = initialCount;
  }
}

This seems like a bit of a critical issue for typing classes; there's no way I can find to create a class state property that's assigned in the constructor without adding undefined to its type.

Reproduction

See description.

Logs

No response

System Info

N/A

Severity

annoyance

@brunnerh
Copy link
Member

Related:

Use of $state in classes may necessitate that no initial value is set as that is provided via the constructor.
The current typing will then force undefined to be part of the type which can lead to errors.

@CaptainCodeman
Copy link

CaptainCodeman commented Jul 29, 2024

try

export class Foo {
	count = $state(0)

	constructor(initialCount: number) {
		this.count = initialCount
	}
}

count: number = $state() is effectively count: number = undefined

@brunnerh
Copy link
Member

brunnerh commented Jul 29, 2024

That's a hack more than anything (and a painful one for more complex types).
Seen some discussion around allowing $state in the constructor, which would address this better.

As a workaround I would probably use a definite assignment assertion:

export class Foo {
	count: number = $state()!;
	...
}

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

count: number = $state() is effectively count: number = undefined

Yes, that's the problem. It adds undefined to the type, which is incorrect -- if it's definitely assigned in the constructor (and TypeScript asserts that unassigned class fields are), it can never be undefined.

That's a hack more than anything (and a painful one for more complex types). Seen some discussion around allowing $state in the constructor, which would address this better.

As a workaround I would probably use a definite assignment assertion:

export class Foo {
	count: number = $state()!;
	...
}

This almost works, but unfortunately disables TypeScript's definite assignment analysis, which is a pretty huge issue -- it means you could easily introduce a logic branch into the constructor that causes your class field to be undefined in some cases.

@Rich-Harris
Copy link
Member

Related: #11116. An alternative to the current design that's been suggested is to allow this.count = $state(0) in the constructor, i.e. what you tried above:

export class Foo {
  count: number;

  constructor(initialCount) {
    this.count = $state(initialCount);
  }
}

It would solve this problem, and make certain other things easier. The big downsides:

  • it's less obvious than requiring the fields to be declared upfront, a la private fields
  • it looks like you should be able to move the assignment into e.g. a this.init() method, but you can't — it needs to be definitely assigned in the constructor (though the TypeScript precedent makes this probably okay)
  • it's confusing that you're doing this.count = ... but not creating a count property on this (you're invoking a setter on the prototype). having class fields be declared up top makes that a little less weird

@robert-puttshack
Copy link

I've been doing this without complaint:

export class Foo {
  count = $state<number>();

  constructor(initialCount) {
    this.count = initialCount;
  }
}

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

@robert-puttshack Doesn't that disable the "this field isn't assigned in the constructor" checks, i.e. this example would not report an error despite being completely type-unsafe (count is undefined)?:

export class Foo {
  count = $state<number>();

  constructor() {
  }
}

@brunnerh
Copy link
Member

This is just the same as as having the type on the left: undefined will be added to the type.
You will see the effect only upon usage, e.g.

export class Foo {
	count = $state<number>();
  
	constructor(initialCount: any) {
	  this.count = initialCount;
	}

	get double() { return this.count * 2; } // Object is possibly 'undefined'. ts(2532)
}

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

Could we do:

class Foo {
  count: number = $state(); // Property 'count' has no initializer and is not definitely assigned in the constructor.(2564)

  constructor() {
  }
}
class Foo {
  count: number | undefined = $state(undefined); // fine, `number` is `undefined`

  constructor() {
  }
}

i.e. for class state fields, you have to explicitly set $state(undefined) to set a default value. Basically all TypeScript would have to see is:

class Foo {
  mustBeAssignedInCtor: number = $state();
  mustBeAssignedInCtor: number; // what TS sees

  defaultUndefined: number | undefined = $state(undefined);
  defaultUndefined: number | undefined = undefined; // what TS sees
}

Not sure of feasibility but this seems like the kind of thing we could provide excellent compiler help with, such as State field `foo` is declared using `$state()` but not assigned in the constructor. If you want to set the default value of `foo` to `undefined`, use `$state(undefined)`

@arxpoetica
Copy link
Member

arxpoetica commented Jul 30, 2024

I'm concerned over the constructor approach, specifically over the second bullet point (.init(), etc.).

I sometimes only initialize my class once and then have lifecycle .setup() and .teardown() functionality on the class. If you want specifics, ask me about my Stripe state management file...lots of complex moving parts and I didn't want it all in a constructor.

UPDATE: I've been persuaded to work with the grain of classes and the strange semantics of constructors. Using $state and $derived inside the constructor might be the least problematic, if you don't have default values.

In many cases, though, I'll still likely do stuff like some_id: string = $state('') as a field declaration above a constructor.

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

Another option: Currently, in class state field definitions, $state() implicitly means "this is a state field with a default value of undefined", i.e.:

class Foo {
  bar: number | undefined = $state();
}

// corresponds to, in TypeScript:
class Foo {
  bar: number | undefined = undefined;
}

But why can't it be this?:

class Foo {
  bar: number = $state();
}

// corresponds to, in TypeScript:
class Foo {
  bar: number; // no default
}

If you do want the class field's default value to be undefined, use $state(undefined), just like you would with any other default value.

@Rich-Harris
Copy link
Member

err... yeah, that seems to work. which I think solves our problem? we'd just need to update this...

declare function $state<T>(initial: T): T;
declare function $state<T>(): T | undefined;

...to this:

declare function $state<T>(initial?: T): T;

Any reason not to do that?

@dummdidumm
Copy link
Member

Because it doesn't help with this issue, and also introduces risk for other runtime bugs:

declare function $state<T>(initial?: T): T;

class X {
    notInitialized: string;
    notActuallyInitializedButTSStaysSilent: string = $state();

    foo() {
        this.notActuallyInitializedButTSStaysSilent.startsWith('x'); // oops, runtime error
    }
}

function foo() {
    let notActuallyInitializedButTSStaysSilent: string = $state(); 

    notActuallyInitializedButTSStaysSilent.startsWith('x'); // oops, runtime error
}

TS playground link

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

Yeah... I guess if we were to go that direction, we'd need a way to strip empty $state() declarations before TypeScript sees them...

@rChaoz
Copy link
Contributor

rChaoz commented Aug 3, 2024

Not sure if feasible but I think something like this could be a solution:

class C {
    notInitialized: $state<number> # ts error...

    constructor() {
    }
}

This doesn't require stripping away something before TS sees it, just define type $state<T> = T. But I think in some cases it might be impossible to avoid the type getting stripped until it reaches Svelte, I'm not sure though.

@webJose
Copy link
Contributor

webJose commented Sep 18, 2024

Hello. I'm surprised that what I do is not listed here. I'll add it to the list hacks built up so far, but as with others, I'd prefer an actual solution.

class A {
    count = $state<number>() as number; // Now the | undefined part is gone.
    constructor(initial: number) {
        count = initial;
    }
}

If you were to ask me, I'd vote for having Svelte allow the use of $state inside the constructor. Cheers.

P.S.: Also allow it in return statements? It would be nice.

@coryvirok
Copy link
Contributor

Hello. I'm surprised that what I do is not listed here. I'll add it to the list hacks built up so far, but as with others, I'd prefer an actual solution.

class A {
    count = $state<number>() as number; // Now the | undefined part is gone.
    constructor(initial: number) {
        count = initial;
    }
}

If you were to ask me, I'd vote for having Svelte allow the use of $state inside the constructor. Cheers.

P.S.: Also allow it in return statements? It would be nice.

Similarly, I assumed there would be a way to do this via an explicit type:

class A {
    count: $State<number>
    constructor(initial: number) {
        count = initial;
    }
}

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

Let it be noted that @webJose's solution does work, but it disables TypeScript's definite-assignment-in-constructor checks, which are quite important.

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

No branches or pull requests

10 participants