Skip to content

Svelte 5: $derived usage in class depending on constructor args is convoluted #11116

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

Open
brunnerh opened this issue Apr 10, 2024 · 15 comments
Open
Milestone

Comments

@brunnerh
Copy link
Member

Describe the problem

When reactively passing data to a class it has to be wrapped in a function or object. The class internally might want to use $derived to access it, but since that is currently only possible in a property initializer, any value passed into the constructor has to be stored in a separate property first.

E.g.

class Doubler {
    #operandFn = $state();
    operand = $derived(this.#operandFn());

    result = $derived(2 * this.operand);

    constructor(getOperand) {
        this.#operandFn = getOperand;
    }
}

let operand = $state(3);
const doubler = new Doubler(() => operand);

REPL

Describe the proposed solution

Possibly allow reactive assignments in constructors:

  class Doubler {
-     #operandFn = $state();
-     operand = $derived(this.#operandFn());
  
      result = $derived(2 * this.operand);
  
      constructor(getOperand) {
-         this.#operandFn = getOperand;
+         this.operand = $derived.by(getOperand);
      }
  }

If the operand does not need its own property:

  class Doubler {
-     #operandFn = $state();
-     operand = $derived(this.#operandFn());
  
-     result = $derived(2 * this.operand);
  
      constructor(getOperand) {
-         this.#operandFn = getOperand;
+         this.result = $derived(2 * getOperand());
      }
  }

(Don't know if this is feasible or really worth it, but wanted to make a note of this.)

Importance

nice to have

@petermakeswebsites
Copy link

There are many advantages to the new reactivity system but the difficulty in passing reactive functions in the way you mention is one of the qualms I have. It's pretty clunky and awkward. I doubt the solution you proposed would be feasible, though. From what I understand, the declaration of reactive variables outside of the constructor is critical. If the compiler were to look for $derives in the constructor, it would have to set up associated getters and setters outside of the constructor. This would be counter-intuitive, and would be complex and affect things like inheritance in unpredictable ways.

I do think it would be important to put our minds together and focus on this. This is a big sticking point for me. I have a suggestion that I'm not claiming is the right way but might help in direction, but it would be to have a mutable $derived value, which would simplify a lot and I think would be fairly easy to integrate. I'll write it out in a new issue and then link it.

@petermakeswebsites
Copy link

#11128

@Rich-Harris
Copy link
Member

Note that if #operandFn is fixed upon creation, it doesn't need to be state. Just don't attempt to read doubler.operand before it's been assigned. So this or even this...

class Doubler {
  #fn;
  result = $derived(2 * this.#fn());
	
  constructor(fn) {
    this.#fn = fn;
  }
}

...works just fine. Does that change the equation here?

Obviously this would be nicer...

class Doubler {
  constructor(fn) {
    this.result = $derived(2 * fn());
  }
}

...but we decided not to do that because it's not free — it's less 'visible' when you have a more complex class, and the syntactical constraints become a bit more ambiguous (you can have this.x = $derived(...) in a class constructor but nowhere else, and it can't be in an if block or a ternary expression or whatever) which feels a bit React hooks-like.

There's no canonically right answer here, just trade-offs. Since could add this.x = $derived(...) later non-breakingly, my inclination is to wait and see how prevalent this pattern is before adding API surface area.

@petermakeswebsites
Copy link

petermakeswebsites commented Apr 11, 2024

For the first option example you gave, although that's prettier, it does give typescript error Property '#fn' is used before its initialization. Could fill a dummy value in, but that's not nice.

Also, in some of the particular cases, because of class inheritance issues and declaration orders, I have to use a $stateful reactive function. I would paste my code here but it's quite complex and would bore people. It would just simplify my life if there was a way to assign a new function to a $derived variable.

In the particular cases I'm referring to, my best solution is actually to do:

class Calculation {
	getResult = $state()
	constructor(fn) {
		this.getResult = fn;
	}
}

REPL

I can get what I need using Calculation.getResult() but then we're back to Solidville, using functions when we actually only care to get a reactive value. As you say, in the realm of trade-offs ... to me, still, currently, this is generally the best trade-off. It keeps my classes tidy at least. I would overall still prefer the ability to assign to a $derived variable with a new reactive function though.

@Rich-Harris
Copy link
Member

Ah, the TypeScript thing is a nuisance. Wouldn't be the first time it's constrained our API design.

I do think it's important for clarity that state/derived fields are declared consistently as state/derived fields, in the same way that private fields must be declared up front (or in the TypeScript case, all fields).

It occurs to me that the closest counterpart to this, where state is initialised in the constructor...

class Counter {
  count = $state()

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

...would be this:

class Doubler {
  result = $derived();

  constructor(fn) {
    this.result = 2 * fn();
  }
}

This is basically what #11128 is asking for, but without any new runes. We would have the same restrictions as #11455, namely that you must assign the expression directly inside constructor and can only do so once. The example above would compile to this:

class Doubler {
  #result = $.source();

  get result() {
    return $.get(this.#result);
  }

  constructor(fn) {
    this.#result.fn = () => 2 * fn();
  }
}

@dummdidumm
Copy link
Member

Are you saying we implement #11128 and as a side effect we solve this constructor problem, or are you saying we take the idea from that issue but only use for this specific constructor case?
Either way I'm not sure what to take from that idea because reassigning a derived may also mean "temporarily make this value something else until it's recalculated again" - i.e. it's not really clear from reading the assignment what's going on.

@Rich-Harris
Copy link
Member

I'm saying we turn this...

class Doubler {
  result = $derived();

  constructor(fn) {
    this.result = 2 * fn();
  }
}

...into this:

class Doubler {
  #result = $.source();

  get result() {
    return $.get(this.#result);
  }

  constructor(fn) {
    this.#result.fn = () => 2 * fn();
  }
}

@dummdidumm
Copy link
Member

Yes, but are we also allowing this then?

let foo = $derived(count * 2);
// ...
foo = count * 2;

If we only allow one but not the other it's very inconsistent, and either way I feel like it's unobvious if reassigning a derived means temporarily setting the value to something else until it's recalculated again or changing the derived formula.

@Rich-Harris
Copy link
Member

No, we wouldn't allow that. It would only be permitted in this case.

Yes it'd be inconsistent but the only way to not introduce any inconsistency at all is to close this issue as wontfix. To me this suggestion is the least worst option, followed by wontfix

@Antonio-Bennett
Copy link

Would $derived.by() also be permitted requiring a function with a return value

@petermakeswebsites
Copy link

I think the simplest way to solve all these problems is to allow re-assignment of $deriveds as dummdidumm touched on above ^.

If a $state is a reactive variable (no deps) that can be re-assigned via $state = 5, then it makes sense for $derived be a reactive expression (potential deps) that can have its expression re-assigned.

And then we no longer have to do any of this sort of stuff of allowing one-time assignments or ts-constructor stuff.

@Rich-Harris
Copy link
Member

We definitely don't want to do that. In the vast majority of cases, if someone assigns to a derived value it's a mistake, and if we start to allow expressions to be reassigned then people will become extremely confused when fahrenheit stops reacting to changes in celsius after you set it to 0:

<script>
  let celsius = $state(0);
  let fahrenheit = $derived(celsius * 9 / 5 + 32);

  function reset_celsius() {
    celsius = 0
  }

  function reset_fahrenheit() {
    // today this is (rightly) a compile error. under this proposal, it would
    // change the expression to `0`, unlinking it from `celsius`
    fahrenheit = 0
  }
</script>

<input type="range" bind:value={celsius} />

<p>{celsius}</p>
<p>{fahrenheit}</p>

<button onclick={reset_celsius}>set celsius to 0</button>
<button onclick={reset_fahrenheit}>set fahrenheit to 0</button>

I'm talking about allowing it in a very specific situation: when you're in the constructor of a class with a $derived field with no initial expression (which would itself be a compile error anywhere else).

@bdmackie
Copy link

bdmackie commented Sep 4, 2024

Motivation: Show others who come here a pattern for getting something working and a low priority muse on how it could work in the future.

As things stand, this is how I've gotten $derived usable in class fields, leaning towards explicitness with nulls.

export class DoublerState {
    num = $state<number | null>(null);
    dbl = $derived<number | null>(this.num !== null ? this.num * 2 : null);

    constructor(num: number) {
        this.num = num;
    }
}

The caveat to the above is that if the derived field depends on data outside the class, it has to be brought in and set as a field to be referred to. So far that hasn't been an issue for me.

Musing: Improving ergonomics to me would mean removing the need to handle nulls/undefined because there are no null use cases from the outside of the class, so something like:

export class DoublerState {
    num = $state<number>();
    dbl = $derived<number>(this.num * 2);

    constructor(num: number) {
        this.num = num;
    }
}

Sorry I'm not aware of your constraints regarding knowledge of constructors having run etc.

For fun I tried this, which feels like a possible compromise, however the compiler errors on this currently.

export class DoublerState2 {
    constructor(
        initialNum: number,
        public num = $state<number>(initialNum),
        public dbl = $derived<number>(this.num * 2)
    ) {}
}

@vozian
Copy link

vozian commented Oct 1, 2024

EDIT: Not a good idea, see the comment below, leaving it here as an example.


One simple solution I found is to just define the class inside a function:

function createSomeClass(param1, param2){
    class StateClass {
          state1 = $state(param1)
          state2 = $derived(this.state1 * param2)
          ...etc        
    }

   return new StateClass()
}

Works nicely for me so far, but I'm wondering what are the drawbacks of this approach 🤔

@brunnerh
Copy link
Member Author

brunnerh commented Oct 1, 2024

It's really not a good idea.

(Which is also why you get a warning if you directly construct a class, which is pretty much what happens here if you just inline the class definition.)

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

Successfully merging a pull request may close this issue.

7 participants