From b08732dfcb55b7e99df2fd644cb61b5b14acdb9e Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Tue, 28 Mar 2023 11:42:19 -0700 Subject: [PATCH 01/13] === BEGIN react-adapter-update === Add test for react-router-dom --- package.json | 2 +- packages/react/package.json | 3 +- packages/react/test/index.test.tsx | 33 +--------------- packages/react/test/react-router.test.tsx | 48 +++++++++++++++++++++++ packages/react/test/utils.ts | 32 +++++++++++++++ pnpm-lock.yaml | 36 +++++++++++++++-- 6 files changed, 118 insertions(+), 36 deletions(-) create mode 100644 packages/react/test/react-router.test.tsx create mode 100644 packages/react/test/utils.ts diff --git a/package.json b/package.json index e1b061ba0..7b964446d 100644 --- a/package.json +++ b/package.json @@ -30,10 +30,10 @@ "license": "MIT", "devDependencies": { "@babel/core": "^7.19.1", + "@babel/plugin-transform-typescript": "^7.19.1", "@babel/preset-env": "^7.19.1", "@babel/preset-react": "^7.18.6", "@babel/preset-typescript": "^7.18.6", - "@babel/plugin-transform-typescript": "^7.19.1", "@changesets/changelog-github": "^0.4.6", "@changesets/cli": "^2.24.2", "@types/chai": "^4.3.3", diff --git a/packages/react/package.json b/packages/react/package.json index 85151ff98..24af0aab8 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -49,6 +49,7 @@ "@types/react-dom": "^18.0.6", "@types/use-sync-external-store": "^0.0.3", "react": "^18.2.0", - "react-dom": "^18.2.0" + "react-dom": "^18.2.0", + "react-router-dom": "^6.9.0" } } diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index d548e9071..d36b3a651 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -18,52 +18,23 @@ import { import { createRoot, Root } from "react-dom/client"; import { renderToStaticMarkup } from "react-dom/server"; -import { act as realAct } from "react-dom/test-utils"; - -// When testing using react's production build, we can't use act (React -// explicitly throws an error in this situation). So instead we'll fake act by -// just waiting 10ms for React's concurrent rerendering to flush. We'll throw a -// helpful error in afterEach if we detect that act() was called but not -// awaited. -const delay = (ms: number) => new Promise(r => setTimeout(r, ms)); - -let acting = false; -async function prodAct(cb: () => void | Promise): Promise { - acting = true; - await cb(); - await delay(10); - acting = false; -} +import { act, checkHangingAct } from "./utils"; describe("@preact/signals-react", () => { let scratch: HTMLDivElement; let root: Root; - let act: typeof realAct; async function render(element: Parameters[0]) { await act(() => root.render(element)); } - before(async () => { - if (process.env.NODE_ENV === "production") { - act = prodAct as typeof realAct; - } else { - act = realAct; - } - }); - beforeEach(() => { scratch = document.createElement("div"); root = createRoot(scratch); }); afterEach(async () => { - if (acting) { - throw new Error( - "Test finished while still acting. Did you await all act() and render() calls?" - ); - } - + checkHangingAct(); await act(() => root.unmount()); }); diff --git a/packages/react/test/react-router.test.tsx b/packages/react/test/react-router.test.tsx new file mode 100644 index 000000000..2b3822541 --- /dev/null +++ b/packages/react/test/react-router.test.tsx @@ -0,0 +1,48 @@ +// @ts-ignore-next-line +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +import { signal } from "@preact/signals-react"; +import { createElement } from "react"; +import { Route, Routes, MemoryRouter } from "react-router-dom"; + +import { createRoot, Root } from "react-dom/client"; +import { act, checkHangingAct } from "./utils"; + +describe("@preact/signals-react", () => { + let scratch: HTMLDivElement; + let root: Root; + async function render(element: Parameters[0]) { + await act(() => root.render(element)); + } + + beforeEach(() => { + scratch = document.createElement("div"); + root = createRoot(scratch); + }); + + afterEach(async () => { + checkHangingAct(); + await act(() => root.unmount()); + }); + + describe("react-router-dom", () => { + it("Route component should render", async () => { + const name = signal("World")!; + + function App() { + return ( + + + Page 1}> + Hello {name}!}> + + + ); + } + + await render(); + + expect(scratch.innerHTML).to.equal("
Hello World!
"); + }); + }); +}); diff --git a/packages/react/test/utils.ts b/packages/react/test/utils.ts new file mode 100644 index 000000000..b342f1a7a --- /dev/null +++ b/packages/react/test/utils.ts @@ -0,0 +1,32 @@ +import { act as realAct } from "react-dom/test-utils"; + +// When testing using react's production build, we can't use act (React +// explicitly throws an error in this situation). So instead we'll fake act by +// just waiting 10ms for React's concurrent rerendering to flush. We'll make a +// best effort to throw a helpful error in afterEach if we detect that act() was +// called but not awaited. +const delay = (ms: number) => new Promise(r => setTimeout(r, ms)); + +let acting = 0; +async function prodActShim(cb: () => void | Promise): Promise { + acting++; + try { + await cb(); + await delay(10); + } finally { + acting--; + } +} + +export function checkHangingAct() { + if (acting > 0) { + throw new Error( + `It appears act() was called but not awaited. This could happen if a test threw an Error or if a test forgot to await a call to act. Make sure to await act() calls in tests.` + ); + } +} + +export const act = + process.env.NODE_ENV === "production" + ? (prodActShim as typeof realAct) + : realAct; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 705a29c99..c253660e8 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1,12 +1,12 @@ lockfileVersion: 5.4 patchedDependencies: - microbundle@0.15.1: - hash: yvstdq4ikeml4yz3a6bi3bgrvu - path: patches/microbundle@0.15.1.patch '@babel/plugin-transform-typescript@7.19.1': hash: tiqrfntt5y3ned567j2lekmz2i path: patches/@babel__plugin-transform-typescript@7.19.1.patch + microbundle@0.15.1: + hash: yvstdq4ikeml4yz3a6bi3bgrvu + path: patches/microbundle@0.15.1.patch importers: @@ -153,6 +153,7 @@ importers: '@types/use-sync-external-store': ^0.0.3 react: ^18.2.0 react-dom: ^18.2.0 + react-router-dom: ^6.9.0 use-sync-external-store: ^1.2.0 dependencies: '@preact/signals-core': link:../core @@ -163,6 +164,7 @@ importers: '@types/use-sync-external-store': 0.0.3 react: 18.2.0 react-dom: 18.2.0_react@18.2.0 + react-router-dom: 6.10.0_biqbaboplfbrettd7655fr4n2y packages: @@ -2061,6 +2063,11 @@ packages: - supports-color dev: true + /@remix-run/router/1.5.0: + resolution: {integrity: sha512-bkUDCp8o1MvFO+qxkODcbhSqRa6P2GXgrGZVpt0dCXNW2HCSCqYI0ZoAqEOSAjRWmmlKcYgFvN4B4S+zo/f8kg==} + engines: {node: '>=14'} + dev: true + /@rollup/plugin-alias/3.1.9_rollup@2.77.2: resolution: {integrity: sha512-QI5fsEvm9bDzt32k39wpOwZhVzRcL5ydcffUHMyLVaVaLeC70I8TJZ17F1z1eMoLu4E/UOcH9BWVkKpIKdrfiw==} engines: {node: '>=8.0.0'} @@ -6232,6 +6239,29 @@ packages: react: 18.2.0 scheduler: 0.23.0 + /react-router-dom/6.10.0_biqbaboplfbrettd7655fr4n2y: + resolution: {integrity: sha512-E5dfxRPuXKJqzwSe/qGcqdwa18QiWC6f3H3cWXM24qj4N0/beCIf/CWTipop2xm7mR0RCS99NnaqPNjHtrAzCg==} + engines: {node: '>=14'} + peerDependencies: + react: '>=16.8' + react-dom: '>=16.8' + dependencies: + '@remix-run/router': 1.5.0 + react: 18.2.0 + react-dom: 18.2.0_react@18.2.0 + react-router: 6.10.0_react@18.2.0 + dev: true + + /react-router/6.10.0_react@18.2.0: + resolution: {integrity: sha512-Nrg0BWpQqrC3ZFFkyewrflCud9dio9ME3ojHCF/WLsprJVzkq3q3UeEhMCAW1dobjeGbWgjNn/PVF6m46ANxXQ==} + engines: {node: '>=14'} + peerDependencies: + react: '>=16.8' + dependencies: + '@remix-run/router': 1.5.0 + react: 18.2.0 + dev: true + /react/18.2.0: resolution: {integrity: sha512-/3IjMdb2L9QbBdWiW5e3P2/npwMBaU9mHCSCUzNln0ZCYbcfTsGbTJrU/kGemdH2IWmB2ioZ+zkxtmq6g09fGQ==} engines: {node: '>=0.10.0'} From a604199baff8e830bbb52299d87f7d8bf75e02a9 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 29 Mar 2023 02:26:38 -0700 Subject: [PATCH 02/13] Add some notes --- packages/react/src/index.ts | 43 +++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 6ca0dfe4e..f36436483 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -31,6 +31,49 @@ const ProxyInstance = new WeakMap< FunctionComponent >(); +// Idea: +// - For Function components: Use CurrentDispatcher to add the signal effect +// store to every component (kinda expensive?). +// - Actually we could probably skip using useSyncExternalStore and just use +// the effect instance directly... Ideally that'd means components that +// don't use any signals incur no persistent memory cost, outside of an +// empty call to useReducer to generate a rerender function. +// +// Though maybe useSyncExternalStore makes it more concurrent mode safe? It +// seems that useSyncExternalStore may be efficient enough if we don't +// allocate more objects (aka the store). Though the +// `pushStoreConsistencyCheck` function would have a object per +// component... and then it'd have to loop through all of them to check if +// a store changed while rendering (if doing non-blocking work? So +// something concurrent related?). useSyncExternalStore probably isn't +// intended to be used on EVERY component. +// +// Conclusion: Let's avoid useSyncExternalStore for now, and bring it if we +// find bugs. +// +// - When it is set to a valid dispatcher (not the invalid one), start a +// preact/signal effect +// - When it is set to the invalid dispatcher (one that throws), stop the +// effect +// - Note: If a component throws, the CurrentDispatcher is reset so we should +// be able to clear our state +// - We need to store the created updater (aka Effect) for each component to +// track which signals are mounted/updated between rerenders (I think)? We +// should just store this in our useReducer instead of a WeakMap? +// - Check if a dispatcher is invalid by checking if the implementation of +// useCallback.length < 2 or if the text of the function contains +// `"warnInvalidHookAccess"` +// - Additional edge cases to be aware of: +// - In dev, React will change the dispatcher inside of useReducer before +// invoking the reducer (solve by using the locking mechanism in +// Jason's prototype?) +// - Some hooks will change the dispatcher (like `use`) while rendering. +// We need to handle this. Perhaps if changing from a valid dispatcher +// to a valid dispatcher, don't reset the effect? +// - Definitely cache all seen dispatchers in a WeakMap to speed look up +// on rerenders +// - For class components: Use CurrentOwner to mimic the above behavior + const SupportsProxy = typeof Proxy === "function"; const ProxyHandlers = { From 0926a8fa4bcc0e083e049910dff0afbb9484fe87 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 29 Mar 2023 18:55:46 -0700 Subject: [PATCH 03/13] Add test for component that uses useReducer --- packages/react/test/index.test.tsx | 43 ++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index d36b3a651..647ea334e 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -11,6 +11,7 @@ import { createElement, forwardRef, useMemo, + useReducer, memo, StrictMode, createRef, @@ -269,6 +270,48 @@ describe("@preact/signals-react", () => { ); } }); + + it("should correctly render components that have useReducer()", async () => { + const count = signal(0); + + let increment: () => void; + const Test = () => { + const [state, dispatch] = useReducer( + (state: number, action: number) => { + return state + action; + }, + 1 + ); + + increment = () => dispatch(1); + + return ( +
+						{state}
+						{count}
+					
+ ); + }; + + let state = 1; + for (let i = 0; i < 3; i++) { + await act(async () => { + count.value += 1; + await render(); + }); + expect(scratch.innerHTML).to.equal( + `
${state}${count.value}
` + ); + } + + await act(() => { + increment(); + state += 1; + }); + expect(scratch.innerHTML).to.equal( + `
${state}${count.value}
` + ); + }); }); describe("useSignalEffect()", () => { From 71a2f479698f4dc6dbb5d4c4903ac5ed08e01677 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 29 Mar 2023 14:47:40 -0700 Subject: [PATCH 04/13] WIP: First pass at new signal react integration --- packages/react/src/index.ts | 278 +++++++++++++++++------------------- 1 file changed, 133 insertions(+), 145 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index f36436483..eb421a490 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -2,14 +2,16 @@ import { useRef, useMemo, useEffect, - Component, - type FunctionComponent, + // @ts-ignore-next-line + // eslint-disable-next-line @typescript-eslint/no-unused-vars + __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED as ReactInternals, type ReactElement, + type useCallback, + type useReducer, } from "react"; import React from "react"; import jsxRuntime from "react/jsx-runtime"; import jsxRuntimeDev from "react/jsx-dev-runtime"; -import { useSyncExternalStore } from "use-sync-external-store/shim/index.js"; import { signal, computed, @@ -24,12 +26,6 @@ export { signal, computed, batch, effect, Signal, type ReadonlySignal }; const Empty = [] as const; const ReactElemType = Symbol.for("react.element"); // https://github.com/facebook/react/blob/346c7d4c43a0717302d446da9e7423a8e28d8996/packages/shared/ReactSymbols.js#L15 -const ReactMemoType = Symbol.for("react.memo"); // https://github.com/facebook/react/blob/346c7d4c43a0717302d446da9e7423a8e28d8996/packages/shared/ReactSymbols.js#L30 -const ReactForwardRefType = Symbol.for("react.forward_ref"); // https://github.com/facebook/react/blob/346c7d4c43a0717302d446da9e7423a8e28d8996/packages/shared/ReactSymbols.js#L25 -const ProxyInstance = new WeakMap< - FunctionComponent, - FunctionComponent ->(); // Idea: // - For Function components: Use CurrentDispatcher to add the signal effect @@ -73,157 +69,149 @@ const ProxyInstance = new WeakMap< // - Definitely cache all seen dispatchers in a WeakMap to speed look up // on rerenders // - For class components: Use CurrentOwner to mimic the above behavior - -const SupportsProxy = typeof Proxy === "function"; - -const ProxyHandlers = { - /** - * This is a function call trap for functional components. - * When this is called, we know it means React did run 'Component()', - * that means we can use any hooks here to setup our effect and store. - * - * With the native Proxy, all other calls such as access/setting to/of properties will - * be forwarded to the target Component, so we don't need to copy the Component's - * own or inherited properties. - * - * @see https://github.com/facebook/react/blob/2d80a0cd690bb5650b6c8a6c079a87b5dc42bd15/packages/react-reconciler/src/ReactFiberHooks.old.js#L460 - */ - apply( - Component: FunctionComponent, - thisArg: any, - argumentsList: Parameters> - ) { - const store = useMemo(createEffectStore, Empty); - - useSyncExternalStore(store.subscribe, store.getSnapshot, store.getSnapshot); - - const stop = store.updater._start(); - - try { - const children = Component.apply(thisArg, argumentsList); - return children; - // eslint-disable-next-line no-useless-catch - } catch (e) { - // Re-throwing promises that'll be handled by suspense - // or an actual error. - throw e; - } finally { - // Stop effects in either case before return or throw, - // Otherwise the effect will leak. - stop(); - } - }, -}; - -function ProxyFunctionalComponent(Component: FunctionComponent) { - return ProxyInstance.get(Component) || WrapWithProxy(Component); +// +// TODO (tests): +// - Test on react.dev and react.production.min.js. +// - Test on a component that uses useReducer (to trigger the edge case where +// dispatcher changes during render) + +interface ReactDispatcher { + useCallback: typeof useCallback; + useReducer: typeof useReducer; } -function WrapWithProxy(Component: FunctionComponent) { - if (SupportsProxy) { - const ProxyComponent = new Proxy(Component, ProxyHandlers); - - ProxyInstance.set(Component, ProxyComponent); - ProxyInstance.set(ProxyComponent, ProxyComponent); - - return ProxyComponent; - } +let finishUpdate: (() => void) | undefined; +const updaterForComponent = new WeakMap(); - /** - * Emulate a Proxy if environment doesn't support it. - * - * @TODO - unlike Proxy, it's not possible to access the type/Component's - * static properties this way. Not sure if we want to copy all statics here. - * Omitting this for now. - * - * @example - works with Proxy, doesn't with wrapped function. - * ``` - * const el = - * el.type.someOwnOrInheritedProperty; - * el.type.defaultProps; - * ``` - */ - const WrappedComponent: FunctionComponent = (...args) => { - return ProxyHandlers.apply(Component, undefined, args); - }; - - ProxyInstance.set(Component, WrappedComponent); - ProxyInstance.set(WrappedComponent, WrappedComponent); - - return WrappedComponent; +function setCurrentUpdater(updater?: Effect) { + // end tracking for the current update: + if (finishUpdate) finishUpdate(); + // start tracking the new update: + finishUpdate = updater && updater._start(); } -/** - * A redux-like store whose store value is a positive 32bit integer (a 'version'). - * - * React subscribes to this store and gets a snapshot of the current 'version', - * whenever the 'version' changes, we tell React it's time to update the component (call 'onStoreChange'). - * - * How we achieve this is by creating a binding with an 'effect', when the `effect._callback' is called, - * we update our store version and tell React to re-render the component ([1] We don't really care when/how React does it). - * - * [1] - * @see https://reactjs.org/docs/hooks-reference.html#usesyncexternalstore - * @see https://github.com/reactjs/rfcs/blob/main/text/0214-use-sync-external-store.md - */ -function createEffectStore() { +function createUpdater(rerender: () => void): Effect { let updater!: Effect; - let version = 0; - let onChangeNotifyReact: (() => void) | undefined; - - let unsubscribe = effect(function (this: Effect) { + effect(function (this: Effect) { updater = this; }); - updater._callback = function () { - version = (version + 1) | 0; - if (onChangeNotifyReact) onChangeNotifyReact(); - }; + updater._callback = rerender; + return updater; +} + +// To track when we are entering and exiting a component render (i.e. before and +// after React renders a component), we track how the dispatcher changes. +// Outside of a component rendering, the dispatcher is set to an instance that +// errors or warns when any hooks are called (this is too prevent hooks from +// being used outside of components). Right before React renders a component, +// the dispatcher is set to a valid one. Right after React finishes rendering a +// component, the dispatcher is set to an erroring one again. +// +// So, we use this getter and setter to monitor the changes to the current +// ReactDispatcher. When the dispatcher changes from an erroring one to a valid +// dispatcher, we assume we are entering a component render. At this point, we +// setup our auto-subscriptions for any signals used in the component. We do +// this by creating an effect and manually starting the effect. We use +// `useReducer` to get access to a `rerender` function that we can use to +// manually trigger a rerender when a signal we've subscribed changes. +// +// When the dispatcher changes from a valid dispatcher to an erroring one, we +// assume we are exiting a component render. At this point we stop the effect. +// +// Some edge cases to be aware of: +// - In development, useReducer changes the dispatcher to an erroring dispatcher +// before invoking the reducer and resets it right after. We need to ensure +// this doesn't trigger our effect logic when we invoke useReducer. We use a +// boolean to track whether we are currently in our useReducer call. +// +// Subsequent calls to useReducer will also change the dispatcher, but by +// storing the updater in a WeakMap keyed by the rerender function, we can +// ensure we don't create a new updater for each call to useReducer. +// +// TODO: Does the above logic actually work? I think it might not because we +// have to invoke useReducer to get the rerender function which only works if +// we always invoke it in the same order. Consider removing the +// `/warnInvalidHookAccess/` check so only the ContextOnlyDispatcher is +// triggers exiting. +// +// - The `use` hook will change the dispatcher to from a valid update dispatcher +// to a valid mount dispatcher in some cases. So just changing the dispatcher +// isn't enough to know if we are exiting a component render. We need to check +// if we are currently in a valid dispatcher and the next one is a erroring +// one to exit. +let lock = false; +const FORCE_UPDATE = () => ({}); +let currentDispatcher: ReactDispatcher | null = null; +Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { + get() { + return currentDispatcher; + }, + set(nextDispatcher: ReactDispatcher) { + if (lock) { + currentDispatcher = nextDispatcher; + return; + } - return { - updater, - subscribe(onStoreChange: () => void) { - onChangeNotifyReact = onStoreChange; + // TODO: Comment these two lines and describe how `use` hook changes the + // dispatcher from a valid one to a different valid one is some situations + const isEnteringComponentRender = + isErroringDispatcher(currentDispatcher) && + !isErroringDispatcher(nextDispatcher); + + // TODO: Will this incorrectly be true if a component uses useReducer? using + // the WeakMap made it not matter... Perhaps we can use the rerender + // function as the key instead of the ReactCurrentOwner into the + // updaterForComponent WeakMap? + const isExitingComponentRender = + !isErroringDispatcher(currentDispatcher) && + isErroringDispatcher(nextDispatcher); + + if (isEnteringComponentRender) { + // useReducer changes the dispatcher to a throwing one while useReducer is + // running in dev mode, so lock our CurrentDispatcher setter to not run + // while our useReducer is running. + lock = true; + // TODO: Consider switching to useSyncExternalStore + const rerender = nextDispatcher.useReducer(FORCE_UPDATE, {})[1]; + lock = false; + + let updater = updaterForComponent.get(rerender); + if (!updater) { + updater = createUpdater(rerender); + updaterForComponent.set(rerender, updater); + } - return function () { - /** - * Rotate to next version when unsubscribing to ensure that components are re-run - * when subscribing again. - * - * In StrictMode, 'memo'-ed components seem to keep a stale snapshot version, so - * don't re-run after subscribing again if the version is the same as last time. - * - * Because we unsubscribe from the effect, the version may not change. We simply - * set a new initial version in case of stale snapshots here. - */ - version = (version + 1) | 0; - onChangeNotifyReact = undefined; - unsubscribe(); - }; - }, - getSnapshot() { - return version; - }, - }; + setCurrentUpdater(updater); + } else if (isExitingComponentRender) { + setCurrentUpdater(); + } + + currentDispatcher = nextDispatcher; + }, +}); + +// We inject a useReducer into every function component via CurrentDispatcher. +// This prevents injecting into anything other than a function component render. +const invalidHookAccessors = new Map(); +function isErroringDispatcher(dispatcher: ReactDispatcher | null) { + // Treat null as the invalid/erroring dispatcher + if (!dispatcher) return true; + + const cached = invalidHookAccessors.get(dispatcher); + if (cached !== undefined) return cached; + + // we only want the real implementation, not the erroring or warning ones + const invalid = + dispatcher.useCallback.length < 2 || + /warnInvalidHookAccess/.test(dispatcher.useCallback as any); + invalidHookAccessors.set(dispatcher, invalid); + return invalid; } function WrapJsx(jsx: T): T { if (typeof jsx !== "function") return jsx; return function (type: any, props: any, ...rest: any[]) { - if (typeof type === "function" && !(type instanceof Component)) { - return jsx.call(jsx, ProxyFunctionalComponent(type), props, ...rest); - } - - if (type && typeof type === "object") { - if (type.$$typeof === ReactMemoType) { - type.type = ProxyFunctionalComponent(type.type); - return jsx.call(jsx, type, props, ...rest); - } else if (type.$$typeof === ReactForwardRefType) { - type.render = ProxyFunctionalComponent(type.render); - return jsx.call(jsx, type, props, ...rest); - } - } - if (typeof type === "string" && props) { for (let i in props) { let v = props[i]; @@ -271,7 +259,7 @@ function Text({ data }: { data: Signal }) { // Decorate Signals so React renders them as components. Object.defineProperties(Signal.prototype, { $$typeof: { configurable: true, value: ReactElemType }, - type: { configurable: true, value: ProxyFunctionalComponent(Text) }, + type: { configurable: true, value: Text }, props: { configurable: true, get() { From ee26a0fff7b6c5b67de8586a30f20bc157effa46 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 29 Mar 2023 19:32:42 -0700 Subject: [PATCH 05/13] Change logic to only use changing to and from ContextOnlyProvider or null as a signal for entering and exiting a component render --- packages/react/src/index.ts | 111 +++++++++++++----------------------- 1 file changed, 39 insertions(+), 72 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index eb421a490..92ab53268 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -47,33 +47,7 @@ const ReactElemType = Symbol.for("react.element"); // https://github.com/faceboo // Conclusion: Let's avoid useSyncExternalStore for now, and bring it if we // find bugs. // -// - When it is set to a valid dispatcher (not the invalid one), start a -// preact/signal effect -// - When it is set to the invalid dispatcher (one that throws), stop the -// effect -// - Note: If a component throws, the CurrentDispatcher is reset so we should -// be able to clear our state -// - We need to store the created updater (aka Effect) for each component to -// track which signals are mounted/updated between rerenders (I think)? We -// should just store this in our useReducer instead of a WeakMap? -// - Check if a dispatcher is invalid by checking if the implementation of -// useCallback.length < 2 or if the text of the function contains -// `"warnInvalidHookAccess"` -// - Additional edge cases to be aware of: -// - In dev, React will change the dispatcher inside of useReducer before -// invoking the reducer (solve by using the locking mechanism in -// Jason's prototype?) -// - Some hooks will change the dispatcher (like `use`) while rendering. -// We need to handle this. Perhaps if changing from a valid dispatcher -// to a valid dispatcher, don't reset the effect? -// - Definitely cache all seen dispatchers in a WeakMap to speed look up -// on rerenders // - For class components: Use CurrentOwner to mimic the above behavior -// -// TODO (tests): -// - Test on react.dev and react.production.min.js. -// - Test on a component that uses useReducer (to trigger the edge case where -// dispatcher changes during render) interface ReactDispatcher { useCallback: typeof useCallback; @@ -105,40 +79,40 @@ function createUpdater(rerender: () => void): Effect { // errors or warns when any hooks are called (this is too prevent hooks from // being used outside of components). Right before React renders a component, // the dispatcher is set to a valid one. Right after React finishes rendering a -// component, the dispatcher is set to an erroring one again. +// component, the dispatcher is set to an erroring one again. This erroring +// dispatcher is called `ContextOnlyDispatcher` in React's source. // // So, we use this getter and setter to monitor the changes to the current -// ReactDispatcher. When the dispatcher changes from an erroring one to a valid -// dispatcher, we assume we are entering a component render. At this point, we -// setup our auto-subscriptions for any signals used in the component. We do -// this by creating an effect and manually starting the effect. We use +// ReactDispatcher. When the dispatcher changes from the ContextOnlyDispatcher +// to a valid dispatcher, we assume we are entering a component render. At this +// point, we setup our auto-subscriptions for any signals used in the component. +// We do this by creating an effect and manually starting the effect. We use // `useReducer` to get access to a `rerender` function that we can use to // manually trigger a rerender when a signal we've subscribed changes. // -// When the dispatcher changes from a valid dispatcher to an erroring one, we -// assume we are exiting a component render. At this point we stop the effect. +// When the dispatcher changes from a valid dispatcher back to the +// ContextOnlyDispatcher, we assume we are exiting a component render. At this +// point we stop the effect. // // Some edge cases to be aware of: -// - In development, useReducer changes the dispatcher to an erroring dispatcher -// before invoking the reducer and resets it right after. We need to ensure -// this doesn't trigger our effect logic when we invoke useReducer. We use a -// boolean to track whether we are currently in our useReducer call. +// - In development, useReducer, useState, and useMemo changes the dispatcher to +// a different erroring dispatcher before invoking the reducer and resets it +// right after. // -// Subsequent calls to useReducer will also change the dispatcher, but by -// storing the updater in a WeakMap keyed by the rerender function, we can -// ensure we don't create a new updater for each call to useReducer. +// When we invoke our own useReducer while entering a component render, we +// need to prevent this change from re-triggering our logic. We do this by +// using a lock to prevent the setter from running while we are in the setter. // -// TODO: Does the above logic actually work? I think it might not because we -// have to invoke useReducer to get the rerender function which only works if -// we always invoke it in the same order. Consider removing the -// `/warnInvalidHookAccess/` check so only the ContextOnlyDispatcher is -// triggers exiting. +// When a Component's function body invokes useReducer, useState, or useMemo, +// this change in dispatcher should not signal that we are exiting a component +// render. We ignore this change cuz this erroring dispatcher does not pass +// the ContextOnlyDispatcher check and so does not affect our logic. // // - The `use` hook will change the dispatcher to from a valid update dispatcher -// to a valid mount dispatcher in some cases. So just changing the dispatcher -// isn't enough to know if we are exiting a component render. We need to check -// if we are currently in a valid dispatcher and the next one is a erroring -// one to exit. +// to a valid mount dispatcher in some cases. Similarly to useReducer +// mentioned above, we should not signal that we are exiting a component +// during this change. Because these other dispatchers do not pass the +// ContextOnlyDispatcher check, they do not affect our logic. let lock = false; const FORCE_UPDATE = () => ({}); let currentDispatcher: ReactDispatcher | null = null; @@ -152,24 +126,15 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { return; } - // TODO: Comment these two lines and describe how `use` hook changes the - // dispatcher from a valid one to a different valid one is some situations const isEnteringComponentRender = - isErroringDispatcher(currentDispatcher) && - !isErroringDispatcher(nextDispatcher); + isContextOnlyDispatcher(currentDispatcher) && + !isContextOnlyDispatcher(nextDispatcher); - // TODO: Will this incorrectly be true if a component uses useReducer? using - // the WeakMap made it not matter... Perhaps we can use the rerender - // function as the key instead of the ReactCurrentOwner into the - // updaterForComponent WeakMap? const isExitingComponentRender = - !isErroringDispatcher(currentDispatcher) && - isErroringDispatcher(nextDispatcher); + !isContextOnlyDispatcher(currentDispatcher) && + isContextOnlyDispatcher(nextDispatcher); if (isEnteringComponentRender) { - // useReducer changes the dispatcher to a throwing one while useReducer is - // running in dev mode, so lock our CurrentDispatcher setter to not run - // while our useReducer is running. lock = true; // TODO: Consider switching to useSyncExternalStore const rerender = nextDispatcher.useReducer(FORCE_UPDATE, {})[1]; @@ -192,20 +157,22 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { // We inject a useReducer into every function component via CurrentDispatcher. // This prevents injecting into anything other than a function component render. -const invalidHookAccessors = new Map(); -function isErroringDispatcher(dispatcher: ReactDispatcher | null) { - // Treat null as the invalid/erroring dispatcher +const dispatcherTypeCache = new Map(); +function isContextOnlyDispatcher(dispatcher: ReactDispatcher | null) { + // Treat null the same as the ContextOnlyDispatcher. if (!dispatcher) return true; - const cached = invalidHookAccessors.get(dispatcher); + const cached = dispatcherTypeCache.get(dispatcher); if (cached !== undefined) return cached; - // we only want the real implementation, not the erroring or warning ones - const invalid = - dispatcher.useCallback.length < 2 || - /warnInvalidHookAccess/.test(dispatcher.useCallback as any); - invalidHookAccessors.set(dispatcher, invalid); - return invalid; + // The ContextOnlyDispatcher sets all the hook implementations to a function + // that takes no arguments and throws and error. Check the number of arguments + // for this dispatcher's useCallback implementation to determine if it is a + // ContextOnlyDispatcher. All other dispatchers, erroring or not, define + // functions with arguments and so fail this check. + const isContextOnlyDispatcher = dispatcher.useCallback.length < 2; + dispatcherTypeCache.set(dispatcher, isContextOnlyDispatcher); + return isContextOnlyDispatcher; } function WrapJsx(jsx: T): T { From 95d77e100a250ebdaa957ba44a2083c71a8a5853 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 29 Mar 2023 22:35:48 -0700 Subject: [PATCH 06/13] Switch out useReducer for useSyncExternalStore --- packages/react/src/index.ts | 124 +++++++++++++++++++++++------------- 1 file changed, 80 insertions(+), 44 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 92ab53268..5da5944ef 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -20,6 +20,7 @@ import { Signal, type ReadonlySignal, } from "@preact/signals-core"; +import { useSyncExternalStore } from "use-sync-external-store/shim/index"; import type { Effect, JsxRuntimeModule } from "./internal"; export { signal, computed, batch, effect, Signal, type ReadonlySignal }; @@ -27,35 +28,14 @@ export { signal, computed, batch, effect, Signal, type ReadonlySignal }; const Empty = [] as const; const ReactElemType = Symbol.for("react.element"); // https://github.com/facebook/react/blob/346c7d4c43a0717302d446da9e7423a8e28d8996/packages/shared/ReactSymbols.js#L15 -// Idea: -// - For Function components: Use CurrentDispatcher to add the signal effect -// store to every component (kinda expensive?). -// - Actually we could probably skip using useSyncExternalStore and just use -// the effect instance directly... Ideally that'd means components that -// don't use any signals incur no persistent memory cost, outside of an -// empty call to useReducer to generate a rerender function. -// -// Though maybe useSyncExternalStore makes it more concurrent mode safe? It -// seems that useSyncExternalStore may be efficient enough if we don't -// allocate more objects (aka the store). Though the -// `pushStoreConsistencyCheck` function would have a object per -// component... and then it'd have to loop through all of them to check if -// a store changed while rendering (if doing non-blocking work? So -// something concurrent related?). useSyncExternalStore probably isn't -// intended to be used on EVERY component. -// -// Conclusion: Let's avoid useSyncExternalStore for now, and bring it if we -// find bugs. -// -// - For class components: Use CurrentOwner to mimic the above behavior - interface ReactDispatcher { + useRef: typeof useRef; useCallback: typeof useCallback; useReducer: typeof useReducer; + useSyncExternalStore: typeof useSyncExternalStore; } let finishUpdate: (() => void) | undefined; -const updaterForComponent = new WeakMap(); function setCurrentUpdater(updater?: Effect) { // end tracking for the current update: @@ -64,13 +44,63 @@ function setCurrentUpdater(updater?: Effect) { finishUpdate = updater && updater._start(); } -function createUpdater(rerender: () => void): Effect { +interface EffectStore { + updater: Effect; + subscribe(onStoreChange: () => void): () => void; + getSnapshot(): number; +} + +/** + * A redux-like store whose store value is a positive 32bit integer (a 'version'). + * + * React subscribes to this store and gets a snapshot of the current 'version', + * whenever the 'version' changes, we tell React it's time to update the component (call 'onStoreChange'). + * + * How we achieve this is by creating a binding with an 'effect', when the `effect._callback' is called, + * we update our store version and tell React to re-render the component ([1] We don't really care when/how React does it). + * + * [1] + * @see https://reactjs.org/docs/hooks-reference.html#usesyncexternalstore + * @see https://github.com/reactjs/rfcs/blob/main/text/0214-use-sync-external-store.md + */ +function createEffectStore(): EffectStore { let updater!: Effect; - effect(function (this: Effect) { + let version = 0; + let onChangeNotifyReact: (() => void) | undefined; + + let unsubscribe = effect(function (this: Effect) { updater = this; }); - updater._callback = rerender; - return updater; + updater._callback = function () { + version = (version + 1) | 0; + if (onChangeNotifyReact) onChangeNotifyReact(); + }; + + return { + updater, + subscribe(onStoreChange) { + onChangeNotifyReact = onStoreChange; + + return function () { + /** + * Rotate to next version when unsubscribing to ensure that components are re-run + * when subscribing again. + * + * In StrictMode, 'memo'-ed components seem to keep a stale snapshot version, so + * don't re-run after subscribing again if the version is the same as last time. + * + * Because we unsubscribe from the effect, the version may not change. We simply + * set a new initial version in case of stale snapshots here. + */ + version = (version + 1) | 0; + onChangeNotifyReact = undefined; + unsubscribe(); + }; + }, + getSnapshot() { + return version; + }, + }; } // To track when we are entering and exiting a component render (i.e. before and @@ -99,9 +129,11 @@ function createUpdater(rerender: () => void): Effect { // a different erroring dispatcher before invoking the reducer and resets it // right after. // -// When we invoke our own useReducer while entering a component render, we -// need to prevent this change from re-triggering our logic. We do this by -// using a lock to prevent the setter from running while we are in the setter. +// The useSyncExternalStore shim will use some of these hooks when we invoke +// it while entering a component render. We need to prevent this dispatcher +// change caused by these hooks from re-triggering our entering logic (it +// would cause an infinite loop if we did not). We do this by using a lock to +// prevent the setter from running while we are in the setter. // // When a Component's function body invokes useReducer, useState, or useMemo, // this change in dispatcher should not signal that we are exiting a component @@ -114,7 +146,6 @@ function createUpdater(rerender: () => void): Effect { // during this change. Because these other dispatchers do not pass the // ContextOnlyDispatcher check, they do not affect our logic. let lock = false; -const FORCE_UPDATE = () => ({}); let currentDispatcher: ReactDispatcher | null = null; Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { get() { @@ -134,29 +165,34 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { !isContextOnlyDispatcher(currentDispatcher) && isContextOnlyDispatcher(nextDispatcher); + // Update the current dispatcher now so the hooks inside of the + // useSyncExternalStore shim get the right dispatcher. + currentDispatcher = nextDispatcher; if (isEnteringComponentRender) { lock = true; - // TODO: Consider switching to useSyncExternalStore - const rerender = nextDispatcher.useReducer(FORCE_UPDATE, {})[1]; - lock = false; - - let updater = updaterForComponent.get(rerender); - if (!updater) { - updater = createUpdater(rerender); - updaterForComponent.set(rerender, updater); + const storeRef = nextDispatcher.useRef(); + if (storeRef.current == null) { + storeRef.current = createEffectStore(); } - setCurrentUpdater(updater); + const store = storeRef.current; + useSyncExternalStore( + store.subscribe, + store.getSnapshot, + store.getSnapshot + ); + lock = false; + + setCurrentUpdater(store.updater); } else if (isExitingComponentRender) { setCurrentUpdater(); } - - currentDispatcher = nextDispatcher; }, }); -// We inject a useReducer into every function component via CurrentDispatcher. -// This prevents injecting into anything other than a function component render. +// We inject a useSyncExternalStore into every function component via +// CurrentDispatcher. This prevents injecting into anything other than a +// function component render. const dispatcherTypeCache = new Map(); function isContextOnlyDispatcher(dispatcher: ReactDispatcher | null) { // Treat null the same as the ContextOnlyDispatcher. From b1586616070bbdf3e620669056bc17b398a75ff9 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 29 Mar 2023 22:53:08 -0700 Subject: [PATCH 07/13] Explicitly detect other invalid dispatchers separately from ContextOnly and valid dispatchers --- packages/react/src/index.ts | 70 ++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 25 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 5da5944ef..c4ff134a2 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -106,19 +106,20 @@ function createEffectStore(): EffectStore { // To track when we are entering and exiting a component render (i.e. before and // after React renders a component), we track how the dispatcher changes. // Outside of a component rendering, the dispatcher is set to an instance that -// errors or warns when any hooks are called (this is too prevent hooks from -// being used outside of components). Right before React renders a component, -// the dispatcher is set to a valid one. Right after React finishes rendering a -// component, the dispatcher is set to an erroring one again. This erroring -// dispatcher is called `ContextOnlyDispatcher` in React's source. +// errors or warns when any hooks are called. This behavior is prevents hooks +// from being used outside of components. Right before React renders a +// component, the dispatcher is set to a valid one. Right after React finishes +// rendering a component, the dispatcher is set to an erroring one again. This +// erroring dispatcher is called the `ContextOnlyDispatcher` in React's source. // -// So, we use this getter and setter to monitor the changes to the current -// ReactDispatcher. When the dispatcher changes from the ContextOnlyDispatcher -// to a valid dispatcher, we assume we are entering a component render. At this -// point, we setup our auto-subscriptions for any signals used in the component. -// We do this by creating an effect and manually starting the effect. We use -// `useReducer` to get access to a `rerender` function that we can use to -// manually trigger a rerender when a signal we've subscribed changes. +// So, we watch the getter and setter on `ReactCurrentDispatcher.current` to +// monitor the changes to the current ReactDispatcher. When the dispatcher +// changes from the ContextOnlyDispatcher to a valid dispatcher, we assume we +// are entering a component render. At this point, we setup our +// auto-subscriptions for any signals used in the component. We do this by +// creating an effect and manually starting the effect. We use +// `useSyncExternalStore` to trigger rerenders on the component when any signals +// it uses changes. // // When the dispatcher changes from a valid dispatcher back to the // ContextOnlyDispatcher, we assume we are exiting a component render. At this @@ -137,13 +138,13 @@ function createEffectStore(): EffectStore { // // When a Component's function body invokes useReducer, useState, or useMemo, // this change in dispatcher should not signal that we are exiting a component -// render. We ignore this change cuz this erroring dispatcher does not pass -// the ContextOnlyDispatcher check and so does not affect our logic. +// render. We ignore this change by detecting these dispatchers as different +// from ContextOnlyDispatcher and other valid dispatchers. // // - The `use` hook will change the dispatcher to from a valid update dispatcher // to a valid mount dispatcher in some cases. Similarly to useReducer // mentioned above, we should not signal that we are exiting a component -// during this change. Because these other dispatchers do not pass the +// during this change. Because these other valid dispatchers do not pass the // ContextOnlyDispatcher check, they do not affect our logic. let lock = false; let currentDispatcher: ReactDispatcher | null = null; @@ -157,13 +158,20 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { return; } + const currentDispatcherType = getDispatcherType(currentDispatcher); + const nextDispatcherType = getDispatcherType(nextDispatcher); + + // We are entering a component render if the current dispatcher is the + // ContextOnlyDispatcher and the next dispatcher is a valid dispatcher. const isEnteringComponentRender = - isContextOnlyDispatcher(currentDispatcher) && - !isContextOnlyDispatcher(nextDispatcher); + currentDispatcherType === ContextOnlyDispatcherType && + nextDispatcherType === ValidDispatcherType; + // We are exiting a component render if the current dispatcher is a valid + // dispatcher and the next dispatcher is the ContextOnlyDispatcher. const isExitingComponentRender = - !isContextOnlyDispatcher(currentDispatcher) && - isContextOnlyDispatcher(nextDispatcher); + currentDispatcherType === ValidDispatcherType && + nextDispatcherType === ContextOnlyDispatcherType; // Update the current dispatcher now so the hooks inside of the // useSyncExternalStore shim get the right dispatcher. @@ -190,13 +198,17 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { }, }); +const ValidDispatcherType = 0; +const ContextOnlyDispatcherType = 1; +const ErroringDispatcherType = 2; + // We inject a useSyncExternalStore into every function component via // CurrentDispatcher. This prevents injecting into anything other than a // function component render. -const dispatcherTypeCache = new Map(); -function isContextOnlyDispatcher(dispatcher: ReactDispatcher | null) { +const dispatcherTypeCache = new Map(); +function getDispatcherType(dispatcher: ReactDispatcher | null): number { // Treat null the same as the ContextOnlyDispatcher. - if (!dispatcher) return true; + if (!dispatcher) return ContextOnlyDispatcherType; const cached = dispatcherTypeCache.get(dispatcher); if (cached !== undefined) return cached; @@ -206,9 +218,17 @@ function isContextOnlyDispatcher(dispatcher: ReactDispatcher | null) { // for this dispatcher's useCallback implementation to determine if it is a // ContextOnlyDispatcher. All other dispatchers, erroring or not, define // functions with arguments and so fail this check. - const isContextOnlyDispatcher = dispatcher.useCallback.length < 2; - dispatcherTypeCache.set(dispatcher, isContextOnlyDispatcher); - return isContextOnlyDispatcher; + let type: number; + if (dispatcher.useCallback.length < 2) { + type = ContextOnlyDispatcherType; + } else if (/Invalid/.test(dispatcher.useCallback as any)) { + type = ErroringDispatcherType; + } else { + type = ValidDispatcherType; + } + + dispatcherTypeCache.set(dispatcher, type); + return type; } function WrapJsx(jsx: T): T { From 40a1ffd21043e366f948f862d1b9f786f108c0a5 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 30 Mar 2023 00:08:40 -0700 Subject: [PATCH 08/13] Add basic useSignal test --- packages/react/test/index.test.tsx | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index 647ea334e..1d9a9ea78 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -6,6 +6,8 @@ import { computed, useComputed, useSignalEffect, + useSignal, + Signal, } from "@preact/signals-react"; import { createElement, @@ -314,6 +316,28 @@ describe("@preact/signals-react", () => { }); }); + describe("useSignal()", () => { + it("should create a signal from a primitive value", async () => { + function App() { + const count = useSignal(1); + return ( +
+ {count} + +
+ ); + } + + await render(); + expect(scratch.textContent).to.equal("1Increment"); + + await act(() => { + scratch.querySelector("button")!.click(); + }); + expect(scratch.textContent).to.equal("2Increment"); + }); + }); + describe("useSignalEffect()", () => { it("should be invoked after commit", async () => { const ref = createRef(); From 5fd438db9793d73343403e8926b9b69b03fb26f9 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 30 Mar 2023 10:33:15 -0700 Subject: [PATCH 09/13] Add changeset --- .changeset/angry-papayas-accept.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/angry-papayas-accept.md diff --git a/.changeset/angry-papayas-accept.md b/.changeset/angry-papayas-accept.md new file mode 100644 index 000000000..e0e957254 --- /dev/null +++ b/.changeset/angry-papayas-accept.md @@ -0,0 +1,5 @@ +--- +"@preact/signals-react": minor +--- + +Revert react integration to tracking current dispatcher From e484b7c70073d339e0ce1f9a200f28dc4e89f8d3 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 30 Mar 2023 15:04:08 -0700 Subject: [PATCH 10/13] Enable local testing with React 16 & 17 by creating a mock createRoot implementation for those versions of React --- packages/react/test/index.test.tsx | 12 ++++---- packages/react/test/react-router.test.tsx | 9 +++--- packages/react/test/utils.ts | 35 +++++++++++++++++++++++ 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index 1d9a9ea78..c68871f01 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -7,7 +7,6 @@ import { useComputed, useSignalEffect, useSignal, - Signal, } from "@preact/signals-react"; import { createElement, @@ -19,9 +18,8 @@ import { createRef, } from "react"; -import { createRoot, Root } from "react-dom/client"; import { renderToStaticMarkup } from "react-dom/server"; -import { act, checkHangingAct } from "./utils"; +import { createRoot, Root, act, checkHangingAct } from "./utils"; describe("@preact/signals-react", () => { let scratch: HTMLDivElement; @@ -31,14 +29,16 @@ describe("@preact/signals-react", () => { await act(() => root.render(element)); } - beforeEach(() => { + beforeEach(async () => { scratch = document.createElement("div"); - root = createRoot(scratch); + document.body.appendChild(scratch); + root = await createRoot(scratch); }); afterEach(async () => { checkHangingAct(); await act(() => root.unmount()); + scratch.remove(); }); describe("Text bindings", () => { @@ -455,12 +455,14 @@ describe("@preact/signals-react", () => { const child = scratch.firstElementChild; + expect(scratch.innerHTML).to.equal("

foo

"); expect(cleanup).not.to.have.been.called; expect(spy).to.have.been.calledOnceWith("foo", child); spy.resetHistory(); await render(null); + expect(scratch.innerHTML).to.equal(""); expect(spy).not.to.have.been.called; expect(cleanup).to.have.been.calledOnce; // @note: React cleans up the ref eagerly, so it's already null by the time the callback runs. diff --git a/packages/react/test/react-router.test.tsx b/packages/react/test/react-router.test.tsx index 2b3822541..2ae49de32 100644 --- a/packages/react/test/react-router.test.tsx +++ b/packages/react/test/react-router.test.tsx @@ -5,8 +5,7 @@ import { signal } from "@preact/signals-react"; import { createElement } from "react"; import { Route, Routes, MemoryRouter } from "react-router-dom"; -import { createRoot, Root } from "react-dom/client"; -import { act, checkHangingAct } from "./utils"; +import { act, checkHangingAct, createRoot, Root } from "./utils"; describe("@preact/signals-react", () => { let scratch: HTMLDivElement; @@ -15,14 +14,16 @@ describe("@preact/signals-react", () => { await act(() => root.render(element)); } - beforeEach(() => { + beforeEach(async () => { scratch = document.createElement("div"); - root = createRoot(scratch); + document.body.appendChild(scratch); + root = await createRoot(scratch); }); afterEach(async () => { checkHangingAct(); await act(() => root.unmount()); + scratch.remove(); }); describe("react-router-dom", () => { diff --git a/packages/react/test/utils.ts b/packages/react/test/utils.ts index b342f1a7a..d893a498b 100644 --- a/packages/react/test/utils.ts +++ b/packages/react/test/utils.ts @@ -1,5 +1,40 @@ import { act as realAct } from "react-dom/test-utils"; +export interface Root { + render(element: JSX.Element | null): void; + unmount(): void; +} + +// We need to use createRoot() if it's available, but it's only available in +// React 18. To enable local testing with React 16 & 17, we'll create a fake +// createRoot() that uses render() and unmountComponentAtNode() instead. +let createRootCache: ((container: Element) => Root) | undefined; +export async function createRoot(container: Element): Promise { + if (!createRootCache) { + try { + // @ts-expect-error ESBuild will replace this import with a require() call + // if it resolves react-dom/client. If it doesn't, it will leave the + // import untouched causing a runtime error we'll handle below. + const { createRoot } = await import("react-dom/client"); + createRootCache = createRoot; + } catch (e) { + // @ts-expect-error ESBuild will replace this import with a require() call + // if it resolves react-dom. + const { render, unmountComponentAtNode } = await import("react-dom"); + createRootCache = (container: Element) => ({ + render(element: JSX.Element) { + render(element, container); + }, + unmount() { + unmountComponentAtNode(container); + }, + }); + } + } + + return createRootCache(container); +} + // When testing using react's production build, we can't use act (React // explicitly throws an error in this situation). So instead we'll fake act by // just waiting 10ms for React's concurrent rerendering to flush. We'll make a From a142a8df4a9b6caa4eec921b5bb8c47757bd772d Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 30 Mar 2023 15:10:45 -0700 Subject: [PATCH 11/13] Move hooks for auto-subscription to a custom hook function to hopefully provide a better devtools experience when the hooks show up in React DevTools --- packages/react/src/index.ts | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index c4ff134a2..cc909b919 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -103,6 +103,22 @@ function createEffectStore(): EffectStore { }; } +/** + * Custom hook to create the effect to track signals used during render and + * subscribe to changes to rerender the component when the signals change + */ +function usePreactSignalStore(nextDispatcher: ReactDispatcher): EffectStore { + const storeRef = nextDispatcher.useRef(); + if (storeRef.current == null) { + storeRef.current = createEffectStore(); + } + + const store = storeRef.current; + useSyncExternalStore(store.subscribe, store.getSnapshot, store.getSnapshot); + + return store; +} + // To track when we are entering and exiting a component render (i.e. before and // after React renders a component), we track how the dispatcher changes. // Outside of a component rendering, the dispatcher is set to an instance that @@ -178,17 +194,7 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { currentDispatcher = nextDispatcher; if (isEnteringComponentRender) { lock = true; - const storeRef = nextDispatcher.useRef(); - if (storeRef.current == null) { - storeRef.current = createEffectStore(); - } - - const store = storeRef.current; - useSyncExternalStore( - store.subscribe, - store.getSnapshot, - store.getSnapshot - ); + const store = usePreactSignalStore(nextDispatcher); lock = false; setCurrentUpdater(store.updater); From 4b304683a52097cf92d51658b4e756a41e3c2dc2 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 30 Mar 2023 15:36:44 -0700 Subject: [PATCH 12/13] Update StrictMode tests to more closely match the issue repro --- packages/react/test/index.test.tsx | 45 +++++++++++++++++------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index c68871f01..1d77198c4 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -212,7 +212,7 @@ describe("@preact/signals-react", () => { }); it("should consistently rerender in strict mode", async () => { - const sig = signal(null!); + const sig = signal(-1); const Test = () =>

{sig.value}

; const App = () => ( @@ -221,18 +221,19 @@ describe("@preact/signals-react", () => { ); - for (let i = 0; i < 3; i++) { - const value = `${i}`; + await render(); + expect(scratch.textContent).to.equal("-1"); + for (let i = 0; i < 3; i++) { await act(async () => { - sig.value = value; - await render(); + sig.value = i; }); - expect(scratch.textContent).to.equal(value); + expect(scratch.textContent).to.equal("" + i); } }); + it("should consistently rerender in strict mode (with memo)", async () => { - const sig = signal(null!); + const sig = signal(-1); const Test = memo(() =>

{sig.value}

); const App = () => ( @@ -241,16 +242,17 @@ describe("@preact/signals-react", () => { ); - for (let i = 0; i < 3; i++) { - const value = `${i}`; + await render(); + expect(scratch.textContent).to.equal("-1"); + for (let i = 0; i < 3; i++) { await act(async () => { - sig.value = value; - await render(); + sig.value = i; }); - expect(scratch.textContent).to.equal(value); + expect(scratch.textContent).to.equal("" + i); } }); + it("should render static markup of a component", async () => { const count = signal(0); @@ -262,10 +264,13 @@ describe("@preact/signals-react", () => { ); }; + + await render(); + expect(scratch.textContent).to.equal("00"); + for (let i = 0; i < 3; i++) { await act(async () => { count.value += 1; - await render(); }); expect(scratch.textContent).to.equal( `${count.value}${count.value}` @@ -282,7 +287,7 @@ describe("@preact/signals-react", () => { (state: number, action: number) => { return state + action; }, - 1 + -2 ); increment = () => dispatch(1); @@ -295,23 +300,25 @@ describe("@preact/signals-react", () => { ); }; - let state = 1; + await render(); + expect(scratch.innerHTML).to.equal( + "
-20
" + ); + for (let i = 0; i < 3; i++) { await act(async () => { count.value += 1; - await render(); }); expect(scratch.innerHTML).to.equal( - `
${state}${count.value}
` + `
-2${count.value}
` ); } await act(() => { increment(); - state += 1; }); expect(scratch.innerHTML).to.equal( - `
${state}${count.value}
` + `
-1${count.value}
` ); }); }); From 465cc0ce939a21a057df2615e8cb05d7333e090d Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 30 Mar 2023 16:32:29 -0700 Subject: [PATCH 13/13] Update useReducer test --- packages/react/test/index.test.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index 1d77198c4..5b4704f08 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -292,10 +292,12 @@ describe("@preact/signals-react", () => { increment = () => dispatch(1); + const doubled = count.value * 2; + return (
 						{state}
-						{count}
+						{doubled}
 					
); }; @@ -310,7 +312,7 @@ describe("@preact/signals-react", () => { count.value += 1; }); expect(scratch.innerHTML).to.equal( - `
-2${count.value}
` + `
-2${count.value * 2}
` ); } @@ -318,7 +320,7 @@ describe("@preact/signals-react", () => { increment(); }); expect(scratch.innerHTML).to.equal( - `
-1${count.value}
` + `
-1${count.value * 2}
` ); }); });