Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@preact/signals-react-transform issues #412

Open
XantreDev opened this issue Sep 14, 2023 · 9 comments
Open

@preact/signals-react-transform issues #412

XantreDev opened this issue Sep 14, 2023 · 9 comments
Labels

Comments

@XantreDev
Copy link
Contributor

XantreDev commented Sep 14, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

XantreDev commented Sep 15, 2023

@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
Copy link
Contributor Author

XantreDev commented Sep 20, 2023

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

andrewiggins added a commit that referenced this issue Sep 29, 2023
"All" mode transforms all components to be reactive to signals. You can still opt-out using `/** @notrackSignals */`.

Related: #412
@XantreDev
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Loque18
Copy link

Loque18 commented Dec 28, 2023

react signals are not reactive for me anymore neither

@XantreDev
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants