preactjs / signals

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

Difference in behaviour of `@preact/signals` and `@preact/signals-react` #383

Closed XantreDev closed 1 year ago

XantreDev commented 1 year ago

Computed that return jsx invalidates Expected behariour: layout updates when computed updates. Current behaviour: layout stays the same

If computed returns jsx - reactivity stops working(preact). I don't know is current behaviour on react is bug or feature, but for me its more like feature. I don't know any reason why computed cannot return jsx.

image

Repro: https://stackblitz.com/edit/vitejs-vite-o1wsux?file=src%2Fapp.tsx

mikerob215 commented 1 year ago

@XantreGodlike this example will work if you add the .value prop to the computeds when you pass them to jsx. The issue isn't that computeds can't return jsx, it's that when you pass the signal directly into jsx, the preact adapter will try to set a text node directly, I think you'd need the component to fully rerender to display jsx. I don't know or use the react adapter so I can't explain the behavior there but I think it might be because the react implementation just does a rerender rather than try to optimize, something like that.

XantreDev commented 1 year ago

Oh interesting but I think it can provide vdom replacements, too.

XantreDev commented 1 year ago

I founded source of issue. In preact adapter we are setting up updater callback for string, but are not checking this it's really will be string

https://github.com/preactjs/signals/blob/9d0cb672b7aa69b8afa22d9b57a7f51de838e2cf/packages/preact/src/index.ts#L84

I think it can be solved this way

    const s = useMemo(() => {
        // mark the parent component as having computeds so it gets optimized
        let v = this.__v;
        while ((v = v.__!)) {
            if (v.__c) {
                v.__c._updateFlags |= HAS_COMPUTEDS;
                break;
            }
        }

        const defaultUpdaterCallback = this._updater!._callback;
        const stringUpdaterCallback = () => {
            (this.base as Text).data = s.peek();
        }; 

        return computed(() => {
            let data = currentSignal.value;
            let s = data.value;
            const result = s === 0 ? 0 : s === true ? "" : s || "";

            if (typeof result === 'object') {
                this._updater!._callback = defaultUpdaterCallback;
            } else {
                // Replace this component's vdom updater with a direct text one:
                this._updater!._callback = stringUpdaterCallback;
            }
            return result;
        });
    }, []);