preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.81k stars 95 forks source link

Dependency array for useComputed #583

Closed foxt closed 4 months ago

foxt commented 4 months ago

A dependency array for useComputed, that whenever any of the members of this array change will force the signal to recompute its value. The primary use case for this would be mixing (P)react props with other Signal values. Take the following example:


import { render } from "preact";
import { useState } from "@preact/compat";
import { signal, computed,useSignal,useComputed } from "@preact/signals";

let colourScheme = signal('light');
const LightColours = {
    red: "#ffaaaa",
    green: "#aaffaa",
    blue: "#aaaaff",
}
const DarkColours = {
    red: "#880000",
    green: "#008800",
    blue: "#000088",
}
let colourSet = computed(() => colourScheme.value == 'dark' ? DarkColours : LightColours)

const ColourDisplay = ({colour}: {colour: string}) => {
    let style = useComputed(() => "color: " + colourSet.value[colour]);
    return <p style={style}>This is some {colour} text.</p>
}

const ColourSelector = () => {
    let [colour,setColour] = useState("red");
    return <div>
        <ColourDisplay colour={colour} />
        <button onClick={() => setColour('red')}>Red</button>
        <button onClick={() => setColour('green')}>Green</button>
        <button onClick={() => setColour('blue')}>Blue</button>
    </div>
}

const App = () => {
    return <>
        <button onClick={() => colourScheme.value = 'light'}>Light</button>
        <button onClick={() => colourScheme.value = 'dark'}>Dark</button>
        <ColourSelector />
        <ColourSelector />
        <ColourSelector />
    </>
}

render(<App />, document.getElementById('app'));

When you click the colour buttons, the text will change as it is directly rendered as part of the component, however the colour of the text will only change when the colour scheme changes as it is a value that the useComputed can subscribe to.

My proposed suggestion is a second parameter to useComputed that takes in an array of dependents, like useEffect

let style = useComputed(
    () => "color: " + colourSet.value[colour],
    [colour]
);

This will now render as the useComputed can check the values of the dependency array for changes. I believe internally useComputed could just pass this straight through to the underlying useMemo.

There is a work around by creating a 'tracker' signal, like so:

const useTrack = <T>(prop: T) => {
    if (prop instanceof Signal) return prop;
    const sig = useSignal(prop)
    useEffect(() => {sig.value = prop}, [prop])
    return sig
}

const ColourDisplay = ({colour}: {colour: string}) => {
    let colourSignal = useTrack(colour);
    let style = useComputed(() => "color: " + colourSet.value[colourSignal.value]);
    return <p style={style}>This is some {colour} text.</p>
}

However this has relatively bad ergonomics as it requires you to keep a second variable around. I'm also not too sure how impactful it would actually be, but I would intuit that this would be less performant than the aforementioned approach.

rschristian commented 4 months ago

Dependency arrays are practically the antithesis of what we want to provide here. They provide a really poor DX and have constantly been the source of complaints, bugs, and poor performance, requiring linters just to wield the API remotely correctly.

The "workaround", if you really need to mix and match signals and other APIs, would be the suggested path. IMO, its ergonomics are far, far better than any dependency array.

The point of the "signal hooks", if you will, is that they're the same core APIs, packaged in a way that avoids some issues with use inside of components. The APIs really shouldn't deviate as it'll lead to confusion.

foxt commented 4 months ago

I do admit, they're not great. But I'm not sure there's any better solution, and of course, different solutions work in different situations, so there should be these entry-hatches that make it easier to deal with this. And plus, while the workaround I mentioned in my original post is relatively easy to implement in your apps, I don't think this would be as easy to without support from library.

I call it a workaround, because it's a fix to a pitfall you have to come across yourself, figure out why it's not working, and come up with your own fix for.

if you really need to

Honestly, I don't understand the way that mixing and matching signals & non signal values is something talked about as if it's an edge case a minority of users will have to deal with, when it's very much not. Every app will have to deal with this anywhere from sometimes to a lot of the time.

For example, if you want a component that takes an arbitrary class property, it has to go from this without Signals

const MyComponent = ({class}: {class?: string}) =>
     <div class={'myComponent ' + (class || '')} />

to

const useTrack = <T>(prop: T) => {
    if (prop instanceof Signal) return prop;
    const sig = useSignal(prop)
    useEffect(() => {sig.value = prop}, [prop])
    return sig
}

const MyComponent = ({class}: {class?: string | Signal<string>}) => {
     let classSignal = useTrack(class);
     return <div class={useComputed(() => 'myComponent ' + (class.value || ''))} />
}

Which, isn't ideal. (if there is actually a better approach PLEASE do tell me because god if this isn't annoying)

rschristian commented 4 months ago

different solutions work in different situations, so there should be these entry-hatches that make it easier to deal with this.

Indeed, but we're quite pleased with the sort of "workaround" as you refer to it. Wrapping & unwrapping signals is pretty simple and TS can do the bulk of the work in most cases. This is the hatch.

because it's a fix to a pitfall you have to come across yourself, figure out why it's not working, and come up with your own fix for.

You were surprised to see non-signal values weren't tracked? That might be a docs issue, but that's a rather fundamental part of the system. It shouldn't really be something you need to spend time figuring out.

Which, isn't ideal. (if there is actually a better approach PLEASE do tell me because god if this isn't annoying)

You don't need to wrap everything in a signal; in fact, you shouldn't, unless you're using it as a reactive value. In your case, where you're just passing a prop in, keep it as a prop.

I'd go with something more like this:

const signalMaybeUnwrapper = (maybeSignal) =>
    maybeSignal instanceof Signal
      ? maybeSignal.peek()
      : maybeSignal;

const MyComponent = (props) => {
     const classVal = signalMaybeUnwrapper(props.class);
     return <div class={'myComponent ' + (classVal || '')} />
}
foxt commented 4 months ago

That approach is pretty nifty, but I like the idea of that when you pass a Signal all the way to the DOM element, any updates will skip any VDOM updates/diffing, which I assume wouldn't work with that.

You were surprised to see non-signal values weren't tracked? That might be a docs issue, but that's a rather fundamental part of the system. It shouldn't really be something you need to spend time figuring out.

Maybe not surprised, but when you're first trying Signals out, get the hang of the basics, and instinctively try to write something like the original code I posted in the OP, and it doesn't work, and you get confused for a second before you look at your code again and realised there's nothing telling Signals about the props change. Then you spend time trying to figure out how to make a Signal that updates with the Props. It's the same kind of mistake you'd make if you, say forget to put something in the dependencies array.

I don't know, maybe the solution would have came naturally to me if I wasn't jumping head first into Hooks and Signals at the first time (...yes, I was mostly writing Class Components up until about 3 months ago)

rschristian commented 4 months ago

That approach is pretty nifty, but I like the idea of that when you pass a Signal all the way to the DOM element, any updates will skip any VDOM updates/diffing, which I assume wouldn't work with that.

I'd approach that on the consumer's side then, only accept signals for your component instead of trying to massage signals & non-signals in the component itself.

Maybe not surprised, but when you're first trying Signals out, get the hang of the basics, and instinctively try to write something like the original code I posted in the OP, and it doesn't work, and you get confused for a second

There may well be a docs issue, something that's unclear or something that needs rewording. Docs are never perfect IMO, so if you have suggestions for how we can make this clearer, we're absolutely interested.

I didn't want to make it sound like I'm dismissing your feedback -- not at all. It's just that this is a fundamental & intentional feature of how the system as a whole works, which we're not likely to change now or in the future.

(...yes, I was mostly writing Class Components up until about 3 months ago)

Class components kick ass, even more so with signals!

import { Component, render } from 'preact';
import { signal } from '@preact/signals'

class Counter extends Component {
    count = signal(0)

    render() {
        return (
            <div class="counter-container">
                <button onClick={() => this.count.value++}>
                    Increment
                </button>
                <input readonly value={this.count} />
                <button onClick={() => this.count.value--}>
                    Decrement
                </button>
            </div>
        );
    }
}

render(<Counter />, document.getElementById('app'));

REPL

I have actually stepped back into using classes far more because of signals, and some others have too.