signals icon indicating copy to clipboard operation
signals copied to clipboard

`@preact/signals-react-transform` issues

Open XantreDev opened this issue 2 years ago • 11 comments

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 avatar Sep 14 '23 15:09 XantreDev

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 avatar Sep 14 '23 15:09 XantreDev

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 avatar Sep 15 '23 07:09 XantreDev

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 avatar Sep 15 '23 07:09 XantreDev

@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 avatar Sep 15 '23 07:09 XantreDev

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 avatar Sep 20 '23 11:09 XantreDev

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 avatar Nov 15 '23 20:11 XantreDev

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

XantreDev avatar Nov 15 '23 21:11 XantreDev

react signals are not reactive for me anymore neither

Loque18 avatar Dec 28 '23 18:12 Loque18

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

XantreDev avatar Dec 28 '23 23:12 XantreDev