preactjs / signals

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

`@preact/signals-react-transform` issues #412

Open XantreDev opened 1 year ago

XantreDev commented 1 year ago

Correct setup:

module.exports = {
  plugins: [
    ['module:@preact/signals-react-transform'],
  ],
}

Preact signals transform using @preact/signals-react, so it still patching react and we will have the same issues (#346 #411). So we should patch @preact/signals-react, because patching of react is inlined here. So just replace function(){Object.defineProperty(n.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactCurrentDispatcher to function(){return;Object.defineProperty(n.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactCurrentDispatcher

App is launching but many places lost reactivity, for example: image Signals cannot be reactively inlined to jsx

XantreDev commented 1 year ago

Not every component uses .value, but uses preact signals with getters for example. I think for easier adoption there should be option to transform all components

XantreDev commented 1 year ago
import { signal } from "@preact/signals-react";

const count = signal(0);

function CounterValue() {
    // Whenever the `count` signal is updated, we'll
    // re-render this component automatically for you
    return <p>Value: {count.value}</p>;
}

After the babel transform runs, it'll look something like:

import { signal, useSignals } from "@preact/signals-react";

const count = signal(0);

function CounterValue() {
    const effect = useSignals();
    try {
        // Whenever the `count` signal is updated, we'll
        // re-render this component automatically for you
        return <p>Value: {count.value}</p>;
    } finally {
        effect.endTracking();
    }
}

The useSignals hook setups the machinery to observe what signals are used inside the component and then automatically re-render the component when those signals change. The endTracking function notifies the tracking mechanism that this component has finished rendering. When your component unmounts, it also unsubscribes from all signals it was using.

This is not true statement, effect actually has no endTracking.

XantreDev commented 1 year ago

Workaround for passing signal into jsx:

import { Signal } from '@preact/signals-react';
import { useSignals } from '@preact/signals-react/runtime';

/** @noTrackSignals */
const SignalValue = ({ data }: { data: Signal }) => {
  const effect = useSignals();
  try {
    return data.value;
  } finally {
    effect.f();
  }
};

Object.defineProperty(Signal.prototype, 'type', {
  configurable: true,
  value: SignalValue,
});
XantreDev commented 1 year ago

@andrewiggins

What to to with this kind of code?

import { ReadonlySignal, Signal } from "@preact-signals/unified-signals";
import { Fn, Objects } from "hotscript";
import { createTransformProps } from "react-fast-hoc";
import { Uncached } from "../$";

export interface WithSignalProp extends Fn {
  return: this["arg1"] extends "children"
    ? this["arg0"]
    : this["arg0"] extends (...args: any[]) => any
    ? this["arg0"]
    : this["arg0"] extends Uncached<any> | ReadonlySignal<any>
    ? never | Uncached<this["arg0"]> | ReadonlySignal<this["arg0"]>
    : this["arg0"] | Uncached<this["arg0"]> | ReadonlySignal<this["arg0"]>;
}

class WithSignalPropsHandler
  implements ProxyHandler<Record<string | symbol, any>>
{
  #valuesCache = new Map<string | symbol, unknown>();

  get(target: Record<string | symbol, any>, p: string | symbol) {
    const value = target[p];
    if (!value) {
      return value;
    }
    if (value instanceof Uncached || value instanceof Signal) {
      return (
        this.#valuesCache.get(p) ??
        this.#valuesCache.set(p, value.value).get(p)!
      );
    }

    return value;
  }
}

/**
 * Allows to pass props to third party components that are not aware of signals. This will subscribe to signals on demand.
 */
export const withSignalProps = createTransformProps<
  [Objects.MapValues<WithSignalProp>]
>((props) => new Proxy(props, new WithSignalPropsHandler()), {
  namePrefix: "Reactified.",
  mimicToNewComponent: false,
});

I'm just wrapping props with proxy and reading signals on demand. How it will perform if I add /** @trackSignals */?

Even if it will clone component in which i applied track signals, there is an issue with destructuring of props.

For example:

/** @trackSignals */
const A = ({a, b}) => <a>{a + b}</a>

Transforms into

/** @trackSignals */
const A = ({a, b}) => {
  // losts reactivity, because `useSignals` used after destructuring
  const effect = useSignals();
  try {
    return (<a>{a + b}</a>)
  } finally {
    effect.f()
  }
}
XantreDev commented 1 year ago

Current implementation cannot handle this case, I think babel transform should wrap your code with some hoc based on Proxies image

Implementation will be something like this image

XantreDev commented 1 year ago

React trying to execute component recursively on third render, so it makes effect called while other effect works

  it("should not crash on signal change while rendering multiple times", async () => {
    // this bug is not occurs in strict mode
    function App() {
      const s = useSignals();
      try {
        const sig = useSignal(0);
        sig.value;
        if (sig.peek() < 100) {
          sig.value += 1;
        }
        return sig.value;
      } finally {
        s.f();
      }
    }

    await render(<App />);

    expect(scratch.innerHTML).to.equal("100");
  });

image

XantreDev commented 1 year ago

So i think we cannot rely on mode without try catches. I think two modes is to complicated to debug image

Loque18 commented 11 months ago

react signals are not reactive for me anymore neither

XantreDev commented 11 months ago

@Loque18 What is the lib version. Seems to be issue was written for 1.3.x version