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

Spring isn't deeply reactive (target cannot be mutated) #14840

Open
nasso opened this issue Dec 26, 2024 · 9 comments
Open

Spring isn't deeply reactive (target cannot be mutated) #14840

nasso opened this issue Dec 26, 2024 · 9 comments

Comments

@nasso
Copy link

nasso commented Dec 26, 2024

Describe the bug

I have something like this in my app:

export class BrokenCounter {
  #spring = new Spring({ value: 0 });

  get count() { return this.#spring.current.value }

  incr() { this.#spring.target.value++ }
  decr() { this.#spring.target.value-- }
}

Rather than re-assigning target to a new object, I directly mutate the target object.

Doing target.value++ instead of target = { value: target.value + 1 } causes the Spring to "miss" the update. It surprised me because I assumed target would be proxied by the Spring (to make it deeply reactive), just like $state. Plus, Spring already "looks deeply" into the target value (to animate individual properties and array items).

Creating a new object and assigning it to target makes for a quick workaround, but it is a bit inefficient and in some situations will lead to a lot of unnecessary garbage being created! This might be a regression from spring, which allowed mutating its target through the update method/function (I think?).

I see three potential solutions to this issue:

  1. Make Spring (and maybe others?) deeply reactive just like $state
  2. Warn against this in the documentation and hope people read it
  3. Somehow detect mutations to target and issue a warning in the console (probably just in development)

Reproduction

https://svelte.dev/playground/b9defdca3842490bb907fcfadaedfecc?version=5.16.0

Logs

No response

System Info

System:
    OS: macOS 15.1.1
    CPU: (8) arm64 Apple M2
    Memory: 72.67 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 22.11.0 - ~/Library/pnpm/node
    Yarn: 1.22.22 - ~/Library/pnpm/yarn
    npm: 10.9.0 - ~/Library/pnpm/npm
    pnpm: 9.14.2 - ~/Library/pnpm/pnpm
    bun: 1.1.2 - ~/.bun/bin/bun
  Browsers:
    Safari: 18.1.1

Severity

annoyance

@webJose
Copy link
Contributor

webJose commented Dec 26, 2024

Fixed:

import { Spring } from "svelte/motion";

export class BrokenCounter { // <---------- No longer broken.
	#counter = $state({ value: 0 });
	#spring = Spring.of(() => this.#counter.value);

	get count() { return this.#spring.current }
	
	incr() { this.#counter.value++ }
	decr() { this.#counter.value-- }
}

export class OkayCounter {
	#spring = new Spring(0);

	get count() { return this.#spring.current }
	
	incr() { this.#spring.target++ }
	decr() { this.#spring.target-- }
}

Explanation: It is bad that the Spring class has been declared as generic because that makes people think that passing anything other than a number would still work. It does not. Springs can only be built to increase or decrease a number. If you nest the number in some arbitrary value, the Spring class cannot possibly know this.

The fix above changes new Spring() for Spring.of(), and then modifies the incr() and decr() methods to correctly bring the spring's target up and down.

@webJose
Copy link
Contributor

webJose commented Dec 26, 2024

For the core team:

In my opinion:

- export class Spring<T> {
+ export class Spring<T extends number> { // Or just remove the generic parameter.
- static of<U>(fn: () => U, options?: SpringOpts): Spring<U>;
+ static of<U extends number>(fn: () => U, options?: SpringOpts): Spring<U>; // Or remove U.

Unsure why Spring/Tween accepts any data type.

Feature suggestion:

// New overload.
static of<U extends number>(getter: () => U, setter: (newValue: U) => void, options?: SpringOpts): Spring<U>;

By adding a setter, people can write to Spring.target.

@nasso
Copy link
Author

nasso commented Dec 26, 2024

Historically, I think spring and tween always had explicit support for number, objects containing number values and even arrays of numbers. This is by-design and is even showcased in the tutorial as well as in this example on the playground.

The primary use-case seems to be for types like { x: number, y: number }, so that a single spring can animate all values together. I actually rely on this in my project.

@webJose
Copy link
Contributor

webJose commented Dec 26, 2024

I wasn't aware of this. My bad. My response was based on the assumption that these could only work with numbers. I'll check the source code...

@webJose
Copy link
Contributor

webJose commented Dec 26, 2024

Ammendment

Best to just get rid of T, I think?

+ type MotionPrimitive = number | Date;
+ interface MotionRecord {
+     [x: string]: MotionPrimitive | MotionRecord | (MotionPrimitive | MotionRecord)[];
+ };

- export class Spring<T>
+ export class Spring {
  ...
-  constructor(value: T, options?: SpringOpts);
+  constructor(value: MotionRecord[''], options?: SpringOpts);

Do similar thing to the static of() method.


As for the bug: Is there any reference in the documents or any official examples that imply that increasing target as if target were a number would work on non-numeric values? If not, the below fixes the issue and the issue is not really a bug:

import { Spring } from "svelte/motion";

export class BrokenCounter {
	#spring = new Spring({ value: 0 });

	get count() { return this.#spring.current.value }
	
	incr() { this.#spring.target = { value: this.#spring.target.value + 1 } }
	decr() { this.#spring.target = { value: this.#spring.target.value - 1 } }
}

export class OkayCounter {
	#spring = new Spring(0);

	get count() { return this.#spring.current }
	
	incr() { this.#spring.target++ }
	decr() { this.#spring.target-- }
}

@nasso
Copy link
Author

nasso commented Dec 27, 2024

I don't think this is a good idea 😅

For type-safety, the exact type needs to be known so that target and current can be of type T.

@webJose
Copy link
Contributor

webJose commented Dec 27, 2024

It is unclear to me what isn't a good idea. The typing I proposed is a mere reflection of what the code is currently capable of processing.

@nasso
Copy link
Author

nasso commented Dec 27, 2024

If Spring is no longer generic, this will be a type error:

const mySpring = new Spring(0);

mySpring.target++; // An arithmetic operand must be of type 'any', 'number', 'bigint' or an enum type. (2356)

[TypeScript Playground link]

Because TypeScript is unable to "remember" that the Spring is currently holding a number.

@webJose
Copy link
Contributor

webJose commented Dec 27, 2024

Ah I see. We then just need Spring<T extends MotionRecord['']>.

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.

2 participants