Optimizing useComputed perf

[JoviDeCroock]

Related to preactjs/signals#772

[github-actions]

Size Change: 0 B

Total Size: 449 kB

ℹ️ View Unchanged

Filename	Size
build/assets/blog-page-********.js	254 B
build/assets/docsearch-********.js	28.9 kB
build/assets/errors-********.js	342 B
build/assets/index-********.js	33.2 kB
build/assets/index-********.css	9.85 kB
build/assets/repl-********.css	1.51 kB
build/assets/repl-********.js	148 kB
build/assets/repl-page-********.js	7.65 kB
build/assets/repl.worker-********.js	213 kB
build/assets/style-********.css	3.07 kB
build/assets/style.module-********.js	159 B
build/assets/tutorial-page-********.js	1.99 kB
build/assets/tutorial-page-********.css	1.76 kB

_{compressed-size-action}

[rschristian]

Not aiming to be performant by default still really strikes me as an inconsistent choice here

[JoviDeCroock]

@rschristian The decision wasn't made with eye on not being performant by default it was made in the essence of giving the user more freedom. You can optimize out of this situation but the alternative, where we only hold on to the initial compute function can't be optimized out of, in case the user leverages props or the identity of a signal changes that's something that is an inherent bug.

In eye of the above, do you think we should revisit the decision then?

[rschristian]

Well that's sort of the problem, no? That you have to explicitly optimize out of this situation (which most folks are going to want by default and, for better or for worse, will show up in benches/articles/whatever) as opposed to optimizing into freedom to swap out signal references on the fly.

I don't know if we should or even can revert at this point, but personally, I love it less and less since merging -- I approved the PR so not taking issue with the merging of it of course, just starting to think I personally was wrong there.

[developit]

another option we could consider is this signature:

// runs only when touched signals change:
useComputed(() => { ... });

// runs when touched signals change OR any of the deps referenced change:
useComputed(() => { ... }, [a, b, c]);

// version that approximates current main behavior:
const callback = () => { ... };
useComputed(callback, [callback]);

// likely most common usage:
const preview = useComputed(() => text.value ?? props.text, [props.text]);

[marvinhagemeister]

I like that idea!

[rschristian]

Adding dep arrays back in is going to undercut most of our messaging over the last few years re:them being fiddly and prone to user error, that would be the absolute worst option IMO.

You'd be back to square one when it comes to getting users to correctly mark dependencies, and worse, we can't piggyback off of React's eslint plugin to tell users when their setups are wrong.

[marvinhagemeister]

fair point too