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

Fix rendering signals as text when using react-transform #439

Merged
merged 4 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fresh-panthers-explain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@preact/signals-react-transform": patch
---

Remove top-level requirement from react-transform
5 changes: 5 additions & 0 deletions .changeset/lovely-moons-beam.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@preact/signals-react": patch
---

Fix rendering signals as text when using react-transform
8 changes: 2 additions & 6 deletions packages/react-transform/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,16 +145,12 @@ function isOptedOutOfSignalTracking(path: NodePath | null): boolean {
function isComponentFunction(path: NodePath<FunctionLike>): boolean {
return (
fnNameStartsWithCapital(path) && // Function name indicates it's a component
getData(path.scope, containsJSX) === true && // Function contains JSX
path.scope.parent === path.scope.getProgramParent() // Function is top-level
getData(path.scope, containsJSX) === true // Function contains JSX
);
}

function isCustomHook(path: NodePath<FunctionLike>): boolean {
return (
fnNameStartsWithUse(path) && // Function name indicates it's a hook
path.scope.parent === path.scope.getProgramParent()
); // Function is top-level
return fnNameStartsWithUse(path); // Function name indicates it's a hook
}

function shouldTransform(
Expand Down
8 changes: 6 additions & 2 deletions packages/react/runtime/src/auto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
import React from "react";
import jsxRuntime from "react/jsx-runtime";
import jsxRuntimeDev from "react/jsx-dev-runtime";
import { EffectStore, useSignals, wrapJsx } from "./index";
import { EffectStore, wrapJsx, _useSignalsImplementation } from "./index";

export interface ReactDispatcher {
useRef: typeof React.useRef;
Expand Down Expand Up @@ -146,11 +146,15 @@ const dispatcherMachinePROD = createMachine({
```
*/

export let isAutoSignalTrackingInstalled = false;

let store: EffectStore | null = null;
let lock = false;
let currentDispatcher: ReactDispatcher | null = null;

function installCurrentDispatcherHook() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function still called on top level of @preact/signals-react so i there is a difference

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR doesn't change the functionality of @preact/signals-react in that importing that package still hooks into internals. Changing that behavior will come later since it is a break change. This PR is starting to setup some infra to eventually make that behavior (hooking into React internals) opt-in. So no difference at the moment for people using @preact/signals-react but that will change eventually.

isAutoSignalTrackingInstalled = true;

Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", {
get() {
return currentDispatcher;
Expand All @@ -171,7 +175,7 @@ function installCurrentDispatcherHook() {
isEnteringComponentRender(currentDispatcherType, nextDispatcherType)
) {
lock = true;
store = useSignals();
store = _useSignalsImplementation();
lock = false;
} else if (
isExitingComponentRender(currentDispatcherType, nextDispatcherType)
Expand Down
39 changes: 37 additions & 2 deletions packages/react/runtime/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { signal, computed, effect, Signal } from "@preact/signals-core";
import { useRef, useMemo, useEffect } from "react";
import { useSyncExternalStore } from "use-sync-external-store/shim/index.js";
import { isAutoSignalTrackingInstalled } from "./auto";

export { installAutoSignalTracking } from "./auto";

const Empty = [] as const;
const ReactElemType = Symbol.for("react.element"); // https://github.com/facebook/react/blob/346c7d4c43a0717302d446da9e7423a8e28d8996/packages/shared/ReactSymbols.js#L15
const noop = () => {};

export function wrapJsx<T>(jsx: T): T {
if (typeof jsx !== "function") return jsx;
Expand Down Expand Up @@ -113,14 +115,37 @@ function createEffectStore(): EffectStore {
};
}

function createEmptyEffectStore(): EffectStore {
return {
effect: {
_sources: undefined,
_callback() {},
_start() {
return noop;
},
_dispose() {},
},
subscribe() {
return noop;
},
getSnapshot() {
return 0;
},
f() {},
[symDispose]() {},
};
}

const emptyEffectStore = createEmptyEffectStore();

let finalCleanup: Promise<void> | undefined;
const _queueMicroTask = Promise.prototype.then.bind(Promise.resolve());

/**
* Custom hook to create the effect to track signals used during render and
* subscribe to changes to rerender the component when the signals change.
*/
export function useSignals(): EffectStore {
export function _useSignalsImplementation(): EffectStore {
clearCurrentStore();
if (!finalCleanup) {
finalCleanup = _queueMicroTask(() => {
Expand All @@ -145,7 +170,12 @@ export function useSignals(): EffectStore {
* A wrapper component that renders a Signal's value directly as a Text node or JSX.
*/
function SignalValue({ data }: { data: Signal }) {
return data.value;
const store = useSignals();
try {
return data.value;
} finally {
store.f();
}
}

// Decorate Signals so React renders them as <SignalValue> components.
Expand All @@ -161,6 +191,11 @@ Object.defineProperties(Signal.prototype, {
ref: { configurable: true, value: null },
});

export function useSignals() {
if (isAutoSignalTrackingInstalled) return emptyEffectStore;
return _useSignalsImplementation();
}

export function useSignal<T>(value: T) {
return useMemo(() => signal<T>(value), Empty);
}
Expand Down
Loading