diff --git a/.changeset/lazy-badgers-happen.md b/.changeset/lazy-badgers-happen.md new file mode 100644 index 000000000..2f876a261 --- /dev/null +++ b/.changeset/lazy-badgers-happen.md @@ -0,0 +1,5 @@ +--- +"@preact/signals-react": patch +--- + +Fix React adapter in SSR and when rerendering diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 0b073c858..fb8f5f5e9 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -42,12 +42,12 @@ jobs: - name: Install dependencies run: pnpm install - - name: Tests - run: pnpm test - - name: Build run: pnpm build + - name: Tests + run: pnpm test + - name: Test production build run: pnpm test:minify diff --git a/package.json b/package.json index 7b964446d..b9f6af5e2 100644 --- a/package.json +++ b/package.json @@ -12,11 +12,16 @@ "postbuild:react": "cd packages/react/dist && mv -f react/src/index.d.ts signals.d.ts && rm -dr react", "postbuild": "node ./scripts/node-13-exports.js", "lint": "eslint 'packages/**/*.{ts,tsx,js,jsx}'", - "test": "cross-env COVERAGE=true karma start karma.conf.js --single-run", - "test:minify": "cross-env COVERAGE=true MINIFY=true karma start karma.conf.js --single-run", - "test:watch": "karma start karma.conf.js --no-single-run", - "test:prod": "cross-env MINIFY=true NODE_ENV=production karma start karma.conf.js --single-run", - "test:prod:watch": "cross-env NODE_ENV=production karma start karma.conf.js --no-single-run", + "test": "pnpm test:karma && pnpm test:mocha", + "test:minify": "pnpm test:karma:minify && pnpm test:mocha", + "test:prod": "pnpm test:karma:prod && pnpm test:mocha:prod", + "test:karma": "cross-env COVERAGE=true karma start karma.conf.js --single-run", + "test:karma:minify": "cross-env COVERAGE=true MINIFY=true karma start karma.conf.js --single-run", + "test:karma:watch": "karma start karma.conf.js --no-single-run", + "test:karma:prod": "cross-env MINIFY=true NODE_ENV=production karma start karma.conf.js --single-run", + "test:karma:prod:watch": "cross-env NODE_ENV=production karma start karma.conf.js --no-single-run", + "test:mocha": "cross-env COVERAGE=true mocha --require packages/react/test/node/setup.js --recursive packages/react/test/node/**.test.tsx", + "test:mocha:prod": "cross-env COVERAGE=true NODE_ENV=production mocha --require packages/react/test/node/setup.js --recursive packages/react/test/node/**.test.tsx", "docs:start": "cd docs && pnpm start", "docs:build": "cd docs && pnpm build", "docs:preview": "cd docs && pnpm preview", @@ -34,6 +39,7 @@ "@babel/preset-env": "^7.19.1", "@babel/preset-react": "^7.18.6", "@babel/preset-typescript": "^7.18.6", + "@babel/register": "^7.21.0", "@changesets/changelog-github": "^0.4.6", "@changesets/cli": "^2.24.2", "@types/chai": "^4.3.3", diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index a172edb2a..f09883fd1 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -6,8 +6,6 @@ import { // 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"; @@ -29,10 +27,12 @@ const Empty = [] as const; const ReactElemType = Symbol.for("react.element"); // https://github.com/facebook/react/blob/346c7d4c43a0717302d446da9e7423a8e28d8996/packages/shared/ReactSymbols.js#L15 interface ReactDispatcher { - useRef: typeof useRef; - useCallback: typeof useCallback; - useReducer: typeof useReducer; - useSyncExternalStore: typeof useSyncExternalStore; + useRef: typeof React.useRef; + useCallback: typeof React.useCallback; + useReducer: typeof React.useReducer; + useSyncExternalStore: typeof React.useSyncExternalStore; + useEffect: typeof React.useEffect; + useImperativeHandle: typeof React.useImperativeHandle; } let finishUpdate: (() => void) | undefined; @@ -119,21 +119,27 @@ function usePreactSignalStore(nextDispatcher: ReactDispatcher): EffectStore { return store; } +// In order for signals to work in React, we need to observe what signals a +// component uses while rendering. To do this, we need to know when a component +// is rendering. To do this, we watch the transition of the +// ReactCurrentDispatcher to know when a component is rerendering. +// // 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 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 +// component, the dispatcher is set to an instance that doesn't warn or error +// and contains the implementations of all hooks. Right after React finishes +// rendering a component, the dispatcher is set to the erroring one again. This // erroring dispatcher is called the `ContextOnlyDispatcher` in React's source. // // 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 +// 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 +// creating an Signal effect and manually starting the Signal effect. We use // `useSyncExternalStore` to trigger rerenders on the component when any signals // it uses changes. // @@ -141,10 +147,16 @@ function usePreactSignalStore(nextDispatcher: ReactDispatcher): EffectStore { // 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, useState, and useMemo changes the dispatcher to -// a different erroring dispatcher before invoking the reducer and resets it -// right after. +// Some additional complexities to be aware of: +// - If a component calls `setState` while rendering, React will re-render the +// component immediately. Before triggering the re-render, React will change +// the dispatcher to the HooksDispatcherOnRerender. When we transition to this +// rerendering adapter, we need to re-trigger our hooks to keep the order of +// hooks the same for every render of a component. +// +// - In development, useReducer, useState, and useMemo change the dispatcher to +// a different warning dispatcher (not ContextOnlyDispatcher) before invoking +// the reducer and resets it right after. // // The useSyncExternalStore shim will use some of these hooks when we invoke // it while entering a component render. We need to prevent this dispatcher @@ -153,15 +165,89 @@ function usePreactSignalStore(nextDispatcher: ReactDispatcher): EffectStore { // 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 -// render. We ignore this change by detecting these dispatchers as different -// from ContextOnlyDispatcher and other valid dispatchers. +// this change in dispatcher should not signal that we are entering or exiting +// a component 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 valid dispatchers do not pass the // ContextOnlyDispatcher check, they do not affect our logic. +// +// - When server rendering, React does not change the dispatcher before and +// after each component render. It sets it once for before the first render +// and once for after the last render. This means that we will not be able to +// detect when we are entering or exiting a component render. This is fine +// because we don't need to detect this for server rendering. A component +// can't trigger async rerenders in SSR so we don't need to track signals. +// +// If a component updates a signal value while rendering during SSR, we will +// not rerender the component because the signal value will synchronously +// change so all reads of the signal further down the tree will see the new +// value. + +/* +Below is a state machine definition for transitions between the various +dispatchers in React's prod build. (It does not include dev time warning +dispatchers which are just always ignored). + +ENTER and EXIT suffixes indicates whether this ReactCurrentDispatcher transition +signals we are entering or exiting a component render, or if it doesn't signal a +change in the component rendering lifecyle (NOOP). + +```js +// Paste this into https://stately.ai/viz to visualize the state machine. +import { createMachine } from "xstate"; + +// ENTER, EXIT, NOOP suffixes indicates whether this ReactCurrentDispatcher +// transition signals we are entering or exiting a component render, or +// if it doesn't signal a change in the component rendering lifecyle (NOOP). + +const dispatcherMachinePROD = createMachine({ + id: "ReactCurrentDispatcher_PROD", + initial: "null", + states: { + null: { + on: { + pushDispatcher: "ContextOnlyDispatcher", + }, + }, + ContextOnlyDispatcher: { + on: { + renderWithHooks_Mount_ENTER: "HooksDispatcherOnMount", + renderWithHooks_Update_ENTER: "HooksDispatcherOnUpdate", + pushDispatcher_NOOP: "ContextOnlyDispatcher", + popDispatcher_NOOP: "ContextOnlyDispatcher", + }, + }, + HooksDispatcherOnMount: { + on: { + renderWithHooksAgain_ENTER: "HooksDispatcherOnRerender", + resetHooksAfterThrow_EXIT: "ContextOnlyDispatcher", + finishRenderingHooks_EXIT: "ContextOnlyDispatcher", + }, + }, + HooksDispatcherOnUpdate: { + on: { + renderWithHooksAgain_ENTER: "HooksDispatcherOnRerender", + resetHooksAfterThrow_EXIT: "ContextOnlyDispatcher", + finishRenderingHooks_EXIT: "ContextOnlyDispatcher", + use_ResumeSuspensedMount_NOOP: "HooksDispatcherOnMount", + }, + }, + HooksDispatcherOnRerender: { + on: { + renderWithHooksAgain_ENTER: "HooksDispatcherOnRerender", + resetHooksAfterThrow_EXIT: "ContextOnlyDispatcher", + finishRenderingHooks_EXIT: "ContextOnlyDispatcher", + }, + }, + }, +}); +``` +*/ + let lock = false; let currentDispatcher: ReactDispatcher | null = null; Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { @@ -177,42 +263,35 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { 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 = - 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 = - currentDispatcherType === ValidDispatcherType && - nextDispatcherType === ContextOnlyDispatcherType; - // Update the current dispatcher now so the hooks inside of the // useSyncExternalStore shim get the right dispatcher. currentDispatcher = nextDispatcher; - if (isEnteringComponentRender) { + if (isEnteringComponentRender(currentDispatcherType, nextDispatcherType)) { lock = true; const store = usePreactSignalStore(nextDispatcher); lock = false; setCurrentUpdater(store.updater); - } else if (isExitingComponentRender) { + } else if ( + isExitingComponentRender(currentDispatcherType, nextDispatcherType) + ) { setCurrentUpdater(); } }, }); -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 getDispatcherType(dispatcher: ReactDispatcher | null): number { +type DispatcherType = number; +const ContextOnlyDispatcherType = 1 << 0; +const WarningDispatcherType = 1 << 1; +const MountDispatcherType = 1 << 2; +const UpdateDispatcherType = 1 << 3; +const RerenderDispatcherType = 1 << 4; +const ServerDispatcherType = 1 << 5; +const BrowserClientDispatcherType = + MountDispatcherType | UpdateDispatcherType | RerenderDispatcherType; + +const dispatcherTypeCache = new Map(); +function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { // Treat null the same as the ContextOnlyDispatcher. if (!dispatcher) return ContextOnlyDispatcherType; @@ -220,23 +299,134 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): number { if (cached !== undefined) return cached; // 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. - let type: number; - if (dispatcher.useCallback.length < 2) { + // that takes no arguments and throws and error. This dispatcher is the only + // dispatcher where useReducer and useEffect will have the same + // implementation. + let type: DispatcherType; + const useCallbackImpl = dispatcher.useCallback.toString(); + if (dispatcher.useReducer === dispatcher.useEffect) { type = ContextOnlyDispatcherType; - } else if (/Invalid/.test(dispatcher.useCallback as any)) { - type = ErroringDispatcherType; + + // @ts-expect-error When server rendering, useEffect and useImperativeHandle + // are both set to noop functions and so have the same implementation. + } else if (dispatcher.useEffect === dispatcher.useImperativeHandle) { + type = ServerDispatcherType; + } else if (/Invalid/.test(useCallbackImpl)) { + // We first check for warning dispatchers because they would also pass some + // of the checks below. + type = WarningDispatcherType; + } else if ( + // The development mount dispatcher invokes a function called + // `mountCallback` whereas the development update/re-render dispatcher + // invokes a function called `updateCallback`. Use that difference to + // determine if we are in a mount or update-like dispatcher in development. + // The production mount dispatcher defines an array of the form [callback, + // deps] whereas update/re-render dispatchers read the array using array + // indices (e.g. `[0]` and `[1]`). Use those differences to determine if we + // are in a mount or update-like dispatcher in production. + /updateCallback/.test(useCallbackImpl) || + (/\[0\]/.test(useCallbackImpl) && /\[1\]/.test(useCallbackImpl)) + ) { + // The update and rerender dispatchers have different implementations for + // useReducer. We'll check it's implementation to determine if this is the + // rerender or update dispatcher. + let useReducerImpl = dispatcher.useReducer.toString(); + if ( + // The development rerender dispatcher invokes a function called + // `rerenderReducer` whereas the update dispatcher invokes a function + // called `updateReducer`. The production rerender dispatcher returns an + // array of the form `[state, dispatch]` whereas the update dispatcher + // returns an array of `[fiber.memoizedState, dispatch]` so we check the + // return statement in the implementation of useReducer to differentiate + // between the two. + /rerenderReducer/.test(useReducerImpl) || + /return\s*\[\w+,/.test(useReducerImpl) + ) { + type = RerenderDispatcherType; + } else { + type = UpdateDispatcherType; + } } else { - type = ValidDispatcherType; + type = MountDispatcherType; } dispatcherTypeCache.set(dispatcher, type); return type; } +function isEnteringComponentRender( + currentDispatcherType: DispatcherType, + nextDispatcherType: DispatcherType +): boolean { + if ( + currentDispatcherType & ContextOnlyDispatcherType && + nextDispatcherType & BrowserClientDispatcherType + ) { + // ## Mount or update (ContextOnlyDispatcher -> ValidDispatcher (Mount or Update)) + // + // If the current dispatcher is the ContextOnlyDispatcher and the next + // dispatcher is a valid dispatcher, we are entering a component render. + return true; + } else if ( + currentDispatcherType & WarningDispatcherType || + nextDispatcherType & WarningDispatcherType + ) { + // ## Warning dispatcher + // + // If the current dispatcher or next dispatcher is an warning dispatcher, + // we are not entering a component render. The current warning dispatchers + // are used to warn when hooks are nested improperly and do not indicate + // entering a new component render. + return false; + } else if (nextDispatcherType & RerenderDispatcherType) { + // Any transition into the rerender dispatcher is the beginning of a + // component render, so we should invoke our hooks. Details below. + // + // ## In-place rerendering (e.g. Mount -> Rerender) + // + // If we are transitioning from the mount, update, or rerender dispatcher to + // the rerender dispatcher (e.g. HooksDispatcherOnMount to + // HooksDispatcherOnRerender), then this component is rerendering due to + // calling setState inside of its function body. We are re-entering a + // component's render method and so we should re-invoke our hooks. + return true; + } else { + // ## Resuming suspended mount edge case (Update -> Mount) + // + // If we are transitioning from the update dispatcher to the mount + // dispatcher, then this component is using the `use` hook and is resuming + // from a mount. We should not re-invoke our hooks in this situation since + // we are not entering a new component render, but instead continuing a + // previous render. + // + // ## Other transitions + // + // For example, Mount -> Mount, Update -> Update, Mount -> Update, any + // transition in and out of invalid dispatchers. + // + // There is no known transition for the following transitions so we default + // to not triggering a re-enter of the component. + // - HooksDispatcherOnMount -> HooksDispatcherOnMount + // - HooksDispatcherOnMount -> HooksDispatcherOnUpdate + // - HooksDispatcherOnUpdate -> HooksDispatcherOnUpdate + return false; + } +} + +/** + * We are exiting a component render if the current dispatcher is a valid + * dispatcher and the next dispatcher is the ContextOnlyDispatcher. + */ +function isExitingComponentRender( + currentDispatcherType: DispatcherType, + nextDispatcherType: DispatcherType +): boolean { + return Boolean( + currentDispatcherType & BrowserClientDispatcherType && + nextDispatcherType & ContextOnlyDispatcherType + ); +} + function WrapJsx(jsx: T): T { if (typeof jsx !== "function") return jsx; diff --git a/packages/react/test/exports.test.tsx b/packages/react/test/browser/exports.test.tsx similarity index 100% rename from packages/react/test/exports.test.tsx rename to packages/react/test/browser/exports.test.tsx diff --git a/packages/react/test/browser/mounts.test.tsx b/packages/react/test/browser/mounts.test.tsx new file mode 100644 index 000000000..af132cac9 --- /dev/null +++ b/packages/react/test/browser/mounts.test.tsx @@ -0,0 +1,32 @@ +import { mountSignalsTests } from "../shared/mounting"; +import { + Root, + createRoot, + act, + getConsoleErrorSpy, + checkConsoleErrorLogs, +} from "../shared/utils"; + +describe("@preact/signals-react mounting", () => { + let scratch: HTMLDivElement; + let root: Root; + + async function render(element: JSX.Element | null): Promise { + await act(() => root.render(element)); + return scratch.innerHTML; + } + + beforeEach(async () => { + scratch = document.createElement("div"); + document.body.appendChild(scratch); + root = await createRoot(scratch); + getConsoleErrorSpy().resetHistory(); + }); + + afterEach(async () => { + scratch.remove(); + checkConsoleErrorLogs(); + }); + + mountSignalsTests(render); +}); diff --git a/packages/react/test/react-router.test.tsx b/packages/react/test/browser/react-router.test.tsx similarity index 95% rename from packages/react/test/react-router.test.tsx rename to packages/react/test/browser/react-router.test.tsx index cf85eac1d..eaa1dd8dc 100644 --- a/packages/react/test/react-router.test.tsx +++ b/packages/react/test/browser/react-router.test.tsx @@ -5,7 +5,7 @@ import { signal } from "@preact/signals-react"; import { createElement } from "react"; import * as ReactRouter from "react-router-dom"; -import { act, checkHangingAct, createRoot, Root } from "./utils"; +import { act, checkHangingAct, createRoot, Root } from "../shared/utils"; const MemoryRouter = ReactRouter.MemoryRouter; const Routes = ReactRouter.Routes diff --git a/packages/react/test/index.test.tsx b/packages/react/test/browser/updates.test.tsx similarity index 79% rename from packages/react/test/index.test.tsx rename to packages/react/test/browser/updates.test.tsx index 5bd2d4d91..436357eeb 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/browser/updates.test.tsx @@ -16,6 +16,9 @@ import { memo, StrictMode, createRef, + useState, + useContext, + createContext, } from "react"; import { renderToStaticMarkup } from "react-dom/server"; @@ -26,9 +29,11 @@ import { checkHangingAct, isReact16, isProd, -} from "./utils"; + getConsoleErrorSpy, + checkConsoleErrorLogs, +} from "../shared/utils"; -describe("@preact/signals-react", () => { +describe("@preact/signals-react updating", () => { let scratch: HTMLDivElement; let root: Root; @@ -40,12 +45,15 @@ describe("@preact/signals-react", () => { scratch = document.createElement("div"); document.body.appendChild(scratch); root = await createRoot(scratch); + getConsoleErrorSpy().resetHistory(); }); afterEach(async () => { - checkHangingAct(); await act(() => root.unmount()); scratch.remove(); + + checkConsoleErrorLogs(); + checkHangingAct(); }); describe("Text bindings", () => { @@ -330,6 +338,102 @@ describe("@preact/signals-react", () => { `
-1${count.value * 2}
` ); }); + + it("should not fail when a component calls setState while rendering", async () => { + let increment: () => void; + function App() { + const [state, setState] = useState(0); + increment = () => setState(state + 1); + + if (state > 0 && state < 2) { + setState(state + 1); + } + + return
{state}
; + } + + await render(); + expect(scratch.innerHTML).to.equal("
0
"); + + await act(() => { + increment(); + }); + expect(scratch.innerHTML).to.equal("
2
"); + }); + + it("should not fail when a component calls setState multiple times while rendering", async () => { + let increment: () => void; + function App() { + const [state, setState] = useState(0); + increment = () => setState(state + 1); + + if (state > 0 && state < 5) { + setState(state + 1); + } + + return
{state}
; + } + + await render(); + expect(scratch.innerHTML).to.equal("
0
"); + + await act(() => { + increment(); + }); + expect(scratch.innerHTML).to.equal("
5
"); + }); + + it("should not fail when a component only uses state-less hooks", async () => { + // This test is suppose to trigger a condition in React where the + // HooksDispatcherOnMountWithHookTypesInDEV is used. This dispatcher is + // used in the development build of React if a component has hook types + // defined but no memoizedState, meaning no stateful hooks (e.g. useState) + // are used. `useContext` is an example of a state-less hook because it + // does not mount any hook state onto the fiber's memoizedState field. + // + // However, as of writing, because our react adapter inserts a + // useSyncExternalStore into all components, all components have memoized + // state and so this condition is never hit. However, I'm leaving the test + // to capture this unique behavior to hopefully catch any errors caused by + // not understanding or handling this in the future. + + const sig = signal(0); + const MyContext = createContext(0); + + function Child() { + const value = useContext(MyContext); + return ( +
+ {sig} {value} +
+ ); + } + + let updateContext: () => void; + function App() { + const [value, setValue] = useState(0); + updateContext = () => setValue(value + 1); + + return ( + + + + ); + } + + await render(); + expect(scratch.innerHTML).to.equal("
0 0
"); + + await act(() => { + sig.value++; + }); + expect(scratch.innerHTML).to.equal("
1 0
"); + + await act(() => { + updateContext(); + }); + expect(scratch.innerHTML).to.equal("
1 1
"); + }); }); describe("useSignal()", () => { @@ -479,7 +583,9 @@ describe("@preact/signals-react", () => { expect(spy).to.have.been.calledOnceWith("foo", child); spy.resetHistory(); - await render(null); + await act(() => { + root.unmount(); + }); expect(scratch.innerHTML).to.equal(""); expect(spy).not.to.have.been.called; diff --git a/packages/react/test/node/renderToStaticMarkup.test.tsx b/packages/react/test/node/renderToStaticMarkup.test.tsx new file mode 100644 index 000000000..572d4026b --- /dev/null +++ b/packages/react/test/node/renderToStaticMarkup.test.tsx @@ -0,0 +1,24 @@ +import { signal, useSignalEffect } from "@preact/signals-react"; +import { expect } from "chai"; +import { createElement } from "react"; +import { renderToStaticMarkup } from "react-dom/server"; +import sinon from "sinon"; +import { mountSignalsTests } from "../shared/mounting"; + +describe("renderToStaticMarkup", () => { + mountSignalsTests(renderToStaticMarkup); + + it("should not invoke useSignalEffect", async () => { + const spy = sinon.spy(); + const sig = signal("foo"); + + function App() { + useSignalEffect(() => spy(sig.value)); + return

{sig.value}

; + } + + const html = await renderToStaticMarkup(); + expect(html).to.equal("

foo

"); + expect(spy.called).to.be.false; + }); +}); diff --git a/packages/react/test/node/setup.js b/packages/react/test/node/setup.js new file mode 100644 index 000000000..210f6fac3 --- /dev/null +++ b/packages/react/test/node/setup.js @@ -0,0 +1,52 @@ +// import register from "@babel/register"; +const register = require("@babel/register").default; + +const coverage = String(process.env.COVERAGE) === "true"; + +// @babel/register doesn't hook into the experimental NodeJS ESM loader API so +// we need all test files to run as CommonJS modules in Node +const env = [ + "@babel/preset-env", + { + targets: { + node: "current", + }, + loose: true, + modules: "commonjs", + }, +]; + +const jsx = [ + "@babel/preset-react", + { + runtime: "classic", + pragma: "createElement", + pragmaFrag: "Fragment", + }, +]; + +const ts = [ + "@babel/preset-typescript", + { + jsxPragma: "createElement", + jsxPragmaFrag: "Fragment", + }, +]; + +register({ + extensions: [".js", ".mjs", ".ts", ".tsx", ".mts", ".mtsx"], + cache: true, + + sourceMaps: "inline", + presets: [ts, jsx, env], + plugins: [ + coverage && [ + "istanbul", + { + // TODO: Currently NodeJS tests always run against dist files. Should we + // change this? + // include: minify ? "**/dist/**/*.js" : "**/src/**/*.{js,jsx,ts,tsx}", + }, + ], + ].filter(Boolean), +}); diff --git a/packages/react/test/shared/mounting.tsx b/packages/react/test/shared/mounting.tsx new file mode 100644 index 000000000..79dfe1aa9 --- /dev/null +++ b/packages/react/test/shared/mounting.tsx @@ -0,0 +1,184 @@ +// @ts-ignore-next-line +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +import { + signal, + computed, + useComputed, + useSignal, +} from "@preact/signals-react"; +import { expect } from "chai"; +import { createElement, useReducer, StrictMode, useState } from "react"; + +import { getConsoleErrorSpy, checkConsoleErrorLogs } from "./utils"; + +export function mountSignalsTests( + render: (element: JSX.Element) => string | Promise +) { + beforeEach(async () => { + getConsoleErrorSpy().resetHistory(); + }); + + afterEach(async () => { + checkConsoleErrorLogs(); + }); + + describe("mount text bindings", () => { + it("should render text without signals", async () => { + const html = await render(test); + expect(html).to.equal("test"); + }); + + it("should render Signals as Text", async () => { + const sig = signal("test"); + const html = await render({sig}); + expect(html).to.equal("test"); + }); + + it("should render computed as Text", async () => { + const sig = signal("test"); + const comp = computed(() => `${sig} ${sig}`); + const html = await render({comp}); + expect(html).to.equal("test test"); + }); + }); + + describe("mount component bindings", () => { + it("should mount component with signals as text", async () => { + const sig = signal("foo"); + + function App() { + const value = sig.value; + return

{value}

; + } + + const html = await render(); + expect(html).to.equal("

foo

"); + }); + + it("should activate signal accessed in render", async () => { + const sig = signal(null); + + function App() { + const arr = useComputed(() => { + // trigger read + sig.value; + + return []; + }); + + const str = arr.value.join(", "); + return

{str}

; + } + + try { + await render(); + } catch (e: any) { + expect.fail(e.stack); + } + }); + + it("should properly mount in strict mode", async () => { + const sig = signal(-1); + + const Test = () =>

{sig.value}

; + const App = () => ( + + + + ); + + const html = await render(); + expect(html).to.equal("

-1

"); + }); + + it("should correctly mount components that have useReducer()", async () => { + const count = signal(0); + + const Test = () => { + const [state] = useReducer((state: number, action: number) => { + return state + action; + }, -2); + + const doubled = count.value * 2; + + return ( +
+						{state}
+						{doubled}
+					
+ ); + }; + + const html = await render(); + expect(html).to.equal("
-20
"); + }); + + it("should not fail when a component calls setState while mounting", async () => { + function App() { + const [state, setState] = useState(0); + if (state == 0) { + setState(1); + } + + return
{state}
; + } + + const html = await render(); + expect(html).to.equal("
1
"); + }); + + it("should not fail when a component calls setState multiple times while mounting", async () => { + function App() { + const [state, setState] = useState(0); + if (state < 5) { + setState(state + 1); + } + + return
{state}
; + } + + const html = await render(); + expect(html).to.equal("
5
"); + }); + }); + + describe("useSignal()", () => { + it("should create a signal from a primitive value", async () => { + function App() { + const count = useSignal(1); + return ( +
+ {count} + +
+ ); + } + + const html = await render(); + expect(html).to.equal("
1
"); + }); + + it("should properly update signal values changed during mount", async () => { + function App() { + const count = useSignal(0); + if (count.value == 0) { + count.value++; + } + + return ( +
+ {count} + +
+ ); + } + + const html = await render(); + expect(html).to.equal("
1
"); + + const html2 = await render(); + expect(html2).to.equal("
1
"); + }); + }); +} diff --git a/packages/react/test/utils.ts b/packages/react/test/shared/utils.ts similarity index 50% rename from packages/react/test/utils.ts rename to packages/react/test/shared/utils.ts index 2ac8645e1..e282deb43 100644 --- a/packages/react/test/utils.ts +++ b/packages/react/test/shared/utils.ts @@ -1,4 +1,5 @@ import React from "react"; +import sinon from "sinon"; import { act as realAct } from "react-dom/test-utils"; export interface Root { @@ -41,17 +42,19 @@ export async function createRoot(container: Element): Promise { // 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)); +// waiting for a requestAnimationFrame and then 10ms for React's concurrent +// rerendering and any effects 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 afterFrame = (ms: number) => + new Promise(r => requestAnimationFrame(() => setTimeout(r, ms))); let acting = 0; async function prodActShim(cb: () => void | Promise): Promise { acting++; try { await cb(); - await delay(10); + await afterFrame(10); } finally { acting--; } @@ -69,3 +72,55 @@ export const act = process.env.NODE_ENV === "production" ? (prodActShim as typeof realAct) : realAct; + +/** + * `console.log` supports formatting strings with `%s` for string substitutions. + * This function accepts a string and additional arguments of values and returns + * a string with the values substituted in. + */ +export function consoleFormat(str: string, ...values: unknown[]): string { + let idx = 0; + return str.replace(/%s/g, () => String(values[idx++])); +} + +declare global { + let errorSpy: sinon.SinonSpy | undefined; +} + +// Only one spy can be active on an object at a time and since all tests share +// the same console object we need to make sure we're only spying on it once. +// We'll use this method to share the spy across all tests. +export function getConsoleErrorSpy(): sinon.SinonSpy { + if (typeof errorSpy === "undefined") { + (globalThis as any).errorSpy = sinon.spy(console, "error"); + } + + return errorSpy!; +} + +const messagesToIgnore = [ + // Ignore errors for timeouts of tests that often happen while debugging + /async tests and hooks,/, + // Ignore React 16 warnings about awaiting `act` calls (warning removed in React 18) + /Do not await the result of calling act/, + // Ignore how chai or mocha uses `console.error` to print out errors + /AssertionError/, +]; + +export function checkConsoleErrorLogs(): void { + const errorSpy = getConsoleErrorSpy(); + if (errorSpy.called) { + let message: string; + if (errorSpy.firstCall.args[0].toString().includes("%s")) { + message = consoleFormat(...errorSpy.firstCall.args); + } else { + message = errorSpy.firstCall.args.join(" "); + } + + if (messagesToIgnore.every(re => re.test(message) === false)) { + expect.fail( + `Console.error was unexpectedly called with this message: \n${message}` + ); + } + } +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6b7779b98..4026a6150 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -17,6 +17,7 @@ importers: '@babel/preset-env': ^7.19.1 '@babel/preset-react': ^7.18.6 '@babel/preset-typescript': ^7.18.6 + '@babel/register': ^7.21.0 '@changesets/changelog-github': ^0.4.6 '@changesets/cli': ^2.24.2 '@types/chai': ^4.3.3 @@ -58,6 +59,7 @@ importers: '@babel/preset-env': 7.19.1_@babel+core@7.19.1 '@babel/preset-react': 7.18.6_@babel+core@7.19.1 '@babel/preset-typescript': 7.18.6_@babel+core@7.19.1 + '@babel/register': 7.21.0_@babel+core@7.19.1 '@changesets/changelog-github': 0.4.6 '@changesets/cli': 2.24.2 '@types/chai': 4.3.3 @@ -1579,6 +1581,20 @@ packages: - supports-color dev: true + /@babel/register/7.21.0_@babel+core@7.19.1: + resolution: {integrity: sha512-9nKsPmYDi5DidAqJaQooxIhsLJiNMkGr8ypQ8Uic7cIox7UCDsM7HuUGxdGT7mSDTYbqzIdsOWzfBton/YJrMw==} + engines: {node: '>=6.9.0'} + peerDependencies: + '@babel/core': ^7.0.0-0 + dependencies: + '@babel/core': 7.19.1 + clone-deep: 4.0.1 + find-cache-dir: 2.1.0 + make-dir: 2.1.0 + pirates: 4.0.5 + source-map-support: 0.5.21 + dev: true + /@babel/runtime/7.18.9: resolution: {integrity: sha512-lkqXDcvlFT5rvEjiu6+QYO+1GXrEHRo2LOtS7E4GtX5ESIZOgepqsZBVIj6Pv+a6zqsya9VCgiK1KAK4BvJDAw==} engines: {node: '>=6.9.0'} @@ -2961,6 +2977,15 @@ packages: wrap-ansi: 7.0.0 dev: true + /clone-deep/4.0.1: + resolution: {integrity: sha512-neHB9xuzh/wk0dIHweyAXv2aPGZIVk3pLMe+/RNzINf17fe0OG96QroktYAUm7SM1PBnzTabaLboqqxDyMU+SQ==} + engines: {node: '>=6'} + dependencies: + is-plain-object: 2.0.4 + kind-of: 6.0.3 + shallow-clone: 3.0.1 + dev: true + /clone/1.0.4: resolution: {integrity: sha512-JQHZ2QMW6l3aH/j6xCqQThY/9OH4D/9ls34cgkUBiEeocRTU04tHfKPBsUK1PqZCUQM7GiA0IIXJSuXHI64Kbg==} engines: {node: '>=0.8'} @@ -4051,6 +4076,15 @@ packages: - supports-color dev: true + /find-cache-dir/2.1.0: + resolution: {integrity: sha512-Tq6PixE0w/VMFfCgbONnkiQIVol/JJL7nRMi20fqzA4NRs9AfeqMGeRdPi3wIhYkxjeBaWh2rxwapn5Tu3IqOQ==} + engines: {node: '>=6'} + dependencies: + commondir: 1.0.1 + make-dir: 2.1.0 + pkg-dir: 3.0.0 + dev: true + /find-cache-dir/3.3.2: resolution: {integrity: sha512-wXZV5emFEjrridIgED11OoUKLxiYjAcqot/NJdAkOhlJ+vGzwhOAfcG5OX1jP+S0PcjEn8bdMJv+g2jwQ3Onig==} engines: {node: '>=8'} @@ -4060,6 +4094,13 @@ packages: pkg-dir: 4.2.0 dev: true + /find-up/3.0.0: + resolution: {integrity: sha512-1yD6RmLI1XBfxugvORwlck6f75tYL+iR0jqwsOrOxMZyGYqUuDhJ0l4AXdO1iX/FTs9cBAMEk1gWSEx1kSbylg==} + engines: {node: '>=6'} + dependencies: + locate-path: 3.0.0 + dev: true + /find-up/4.1.0: resolution: {integrity: sha512-PpOwAdQ/YlXQ2vj8a3h8IipDuYRi3wceVQQGYWxNINccq40Anw7BlsEXCMbt1Zt+OLA6Fq9suIpIWD0OsnISlw==} engines: {node: '>=8'} @@ -4598,6 +4639,13 @@ packages: engines: {node: '>=8'} dev: true + /is-plain-object/2.0.4: + resolution: {integrity: sha512-h5PpgXkWitc38BBMYawTYMWJHFZJVnBquFE57xFpjB8pJFiF6gZ+bU+WyI/yqXiFR5mdLsgYNaPe8uao6Uv9Og==} + engines: {node: '>=0.10.0'} + dependencies: + isobject: 3.0.1 + dev: true + /is-reference/1.2.1: resolution: {integrity: sha512-U82MsXXiFIrjCK4otLT+o2NA2Cd2g5MLoOVXUZjIOhLurrRxpEXzI8O0KZHr3IjLvlAH1kTPYSuqer5T9ZVBKQ==} dependencies: @@ -4680,6 +4728,11 @@ packages: resolution: {integrity: sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw==} dev: true + /isobject/3.0.1: + resolution: {integrity: sha512-WhB9zCku7EGTj/HQQRz5aUQEUeoQZH2bWcltRErOpymJ4boYE6wL9Tbr23krRPSZ+C5zqNSrSw+Cc7sZZ4b7vg==} + engines: {node: '>=0.10.0'} + dev: true + /istanbul-lib-coverage/3.2.0: resolution: {integrity: sha512-eOeJ5BHCmHYvQK7xt9GkdHuzuCGS1Y6g9Gvnx3Ym33fz/HpLRYxiS0wHNr+m/MBC8B647Xt608vCDEvhl9c6Mw==} engines: {node: '>=8'} @@ -5008,6 +5061,14 @@ packages: engines: {node: '>= 12.13.0'} dev: true + /locate-path/3.0.0: + resolution: {integrity: sha512-7AO748wWnIhNqAuaty2ZWHkQHRSNfPVIsPIfwEOWO22AmaoVrWavlOcMR5nzTLNYvp36X220/maaRsrec1G65A==} + engines: {node: '>=6'} + dependencies: + p-locate: 3.0.0 + path-exists: 3.0.0 + dev: true + /locate-path/5.0.0: resolution: {integrity: sha512-t7hw9pI+WvuwNJXwk5zVHpyhIqzg2qTlklJOf0mVxGSbe3Fp2VieZcduNYjaLDoy6p9uGpQEGWG87WpMKlNq8g==} engines: {node: '>=8'} @@ -5124,6 +5185,14 @@ packages: sourcemap-codec: 1.4.8 dev: true + /make-dir/2.1.0: + resolution: {integrity: sha512-LS9X+dc8KLxXCb8dni79fLIIUA5VyZoyjSMCwTluaXA0o27cCK0bhXkpgw+sTXVpPy/lSO57ilRixqk0vDmtRA==} + engines: {node: '>=6'} + dependencies: + pify: 4.0.1 + semver: 5.7.1 + dev: true + /make-dir/3.1.0: resolution: {integrity: sha512-g3FeP20LNwhALb/6Cz6Dd4F2ngze0jz7tbzrD2wAV+o9FeNHe4rL+yK2md0J/fiSf1sa1ADhXqi5+oVwOM/eGw==} engines: {node: '>=8'} @@ -5574,6 +5643,13 @@ packages: yocto-queue: 0.1.0 dev: true + /p-locate/3.0.0: + resolution: {integrity: sha512-x+12w/To+4GFfgJhBEpiDcLozRJGegY+Ei7/z0tSLkMmxGZNybVMSfWj9aJn8Z5Fc7dBUNJOOVgPv2H7IwulSQ==} + engines: {node: '>=6'} + dependencies: + p-limit: 2.3.0 + dev: true + /p-locate/4.1.0: resolution: {integrity: sha512-R79ZZ/0wAxKGu3oYMlz8jy/kbhsNrS7SKZ7PxEHBgJ5+F2mtFW2fK2cOtBh1cHYkQsbzFV7I+EoRKe6Yt0oK7A==} engines: {node: '>=8'} @@ -5642,6 +5718,11 @@ packages: engines: {node: '>= 0.8'} dev: true + /path-exists/3.0.0: + resolution: {integrity: sha512-bpC7GYwiDYQ4wYLe+FA8lhRjhQCMcQGuSgGGqDkg/QerRWw9CmGRT0iSOVRSZJ29NMLZgIzqaljJ63oaL4NIJQ==} + engines: {node: '>=4'} + dev: true + /path-exists/4.0.0: resolution: {integrity: sha512-ak9Qy5Q7jYb2Wwcey5Fpvg2KoAc/ZIhLSLOSBmRmygPsGwkVVt0fZa0qrtMz+m6tJTAHfZQ8FnmB4MG4LWy7/w==} engines: {node: '>=8'} @@ -5706,6 +5787,18 @@ packages: engines: {node: '>=10'} dev: true + /pirates/4.0.5: + resolution: {integrity: sha512-8V9+HQPupnaXMA23c5hvl69zXvTwTzyAYasnkb0Tts4XvO4CliqONMOnvlq26rkhLC3nWDFBJf73LU1e1VZLaQ==} + engines: {node: '>= 6'} + dev: true + + /pkg-dir/3.0.0: + resolution: {integrity: sha512-/E57AYkoeQ25qkxMj5PBOVgF8Kiu/h7cYS30Z5+R7WaiCCBfLq58ZI/dSeaEKb9WVJV5n/03QwrN3IeWIFllvw==} + engines: {node: '>=6'} + dependencies: + find-up: 3.0.0 + dev: true + /pkg-dir/4.2.0: resolution: {integrity: sha512-HRDzbaKjC+AOWVXxAU/x54COGeIv9eb+6CkDSQoNTt4XyWoIJvuPsXizxu/Fr23EiekbtZwmh1IcIG/l/a10GQ==} engines: {node: '>=8'} @@ -6590,6 +6683,13 @@ packages: resolution: {integrity: sha512-E5LDX7Wrp85Kil5bhZv46j8jOeboKq5JMmYM3gVGdGH8xFpPWXUMsNrlODCrkoxMEeNi/XZIwuRvY4XNwYMJpw==} dev: true + /shallow-clone/3.0.1: + resolution: {integrity: sha512-/6KqX+GVUdqPuPPd2LxDDxzX6CAbjJehAAOKlNpqqUpAqPM6HeL8f+o3a+JsyGjn2lv0WY8UsTgUJjU9Ok55NA==} + engines: {node: '>=8'} + dependencies: + kind-of: 6.0.3 + dev: true + /shebang-command/1.2.0: resolution: {integrity: sha512-EV3L1+UQWGor21OmnvojK36mhg+TyIKDh3iFBKBohr5xeXIhNBcx8oWdgkTEEQ+BEFFYdLRuqMfd5L84N1V5Vg==} engines: {node: '>=0.10.0'}