diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 8e06c5905..2bad355bb 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -28,16 +28,16 @@ A reproducible example from an online editor such as CodeSandbox or StackBlitz **Desktop (please complete the following information):** -- OS: [e.g. iOS] -- Browser [e.g. chrome, safari] -- Version [e.g. 22] +- OS: [e.g. iOS] +- Browser [e.g. chrome, safari] +- Version [e.g. 22] **Smartphone (please complete the following information):** -- Device: [e.g. iPhone6] -- OS: [e.g. iOS8.1] -- Browser [e.g. stock browser, safari] -- Version [e.g. 22] +- Device: [e.g. iPhone6] +- OS: [e.g. iOS8.1] +- Browser [e.g. stock browser, safari] +- Version [e.g. 22] **Additional context** Add any other context about the problem here. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index b5c694a63..1e393e0f0 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -26,10 +26,10 @@ _Use this section to describe how you tested your changes; if you haven't includ ## Checklist: -- [ ] I have completed the above PR template -- [ ] I have commented my code, particularly in hard-to-understand areas -- [ ] I have made corresponding changes to the documentation (if applicable) -- [ ] My changes generate no new warnings -- [ ] My changes include tests that prove my fix is effective (or that my feature works as intended) -- [ ] New and existing unit/integration tests pass locally with my changes -- [ ] Any dependent changes have corresponding PRs that are listed above +- [ ] I have completed the above PR template +- [ ] I have commented my code, particularly in hard-to-understand areas +- [ ] I have made corresponding changes to the documentation (if applicable) +- [ ] My changes generate no new warnings +- [ ] My changes include tests that prove my fix is effective (or that my feature works as intended) +- [ ] New and existing unit/integration tests pass locally with my changes +- [ ] Any dependent changes have corresponding PRs that are listed above diff --git a/CHANGELOG.md b/CHANGELOG.md index 52de664f6..8eff5dba7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # @formio/react +## 6.2.2 + +### Patch Changes + +- 4c7e7ec: FIO-12286: subscribe the `
` `onAny` handler once per form instance and detach it through the emitter rather than through the already-destroyed instance, fixing duplicate submission records written by leaked `any` listeners; note that `` now always attaches a listener, where before it attached one only if at least one handler prop was passed. + +## 6.2.2-api912.0 + +### Patch Changes + +- 4c7e7ec: FIO-12286: subscribe the `` `onAny` handler once per form instance and detach it through the emitter rather than through the already-destroyed instance, fixing duplicate submission records written by leaked `any` listeners; note that `` now always attaches a listener, where before it attached one only if at least one handler prop was passed. + ## 6.2.1 ### Patch Changes diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..fcdd94c1c --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,62 @@ +# @formio/react + +## Identity + +- **Path:** `packages/react`. **Published name:** `@formio/react`, version in [`package.json`](./package.json). +- **License:** MIT. **OSS sync:** YES — `ossRepo: { repo: github.com/formio/react, srcPath: "." }`. Everything in `src/` ships publicly. +- **Module/language:** TypeScript (CJS `main: lib/index.js`), mixed era. Targets React 17–19 (`peerDependencies: react/react-dom >=17`; `@types/react ^19`). +- **Purpose:** the React wrapper around the `@formio/js` renderer + `@formio/core` data engine. It owns the React surface (components, context, hooks) and the **lifecycle bridge** from React's re-render model to the imperative `@formio/js` instance — **not** rendering/validation/submission, which live in `@formio/js`. + +## Floor — immutable musts + +- **Never widen a create-effect's dependency array to react to a data prop.** Anything in `Form.tsx`'s `[formConstructor, formReadyCallback, formSource, options, url]` or `FormBuilder.tsx`'s `[Builder, initialForm, onBuilderReady, options]` triggers a full instance destroy+recreate. Apply data changes (`submission`) to the live instance in a separate `isEqual`-guarded effect instead. See [react/create-effect-deps-rebuild-02](../../docs/gotchas/react.md#reactcreate-effect-deps-rebuild-02--the-create-effect-dependency-array-rebuilds-the-whole-instance) (markers `G-RC03`/`G-RC04`). +- **Guard every `await instance.ready` against mount/unmount/redraw.** Keep the `isMounted` ref check after the await and destroy orphaned/pending instances (`G-RC01`/`G-RC02`). After `destroy(true)`, instance fields are _deleted_ — optional-chain in any continuation. See [react/async-create-race-01](../../docs/gotchas/react.md#reactasync-create-race-01--async-instance-creation-races-react-mountunmountredraw). +- **Clone shared default props before handing them to a builder/form.** Module-level `const DEFAULT_X = {}` defaults are a single shared reference; the builder mutates the form definition in place. Use `fastCloneDeep`. See [react/shared-mutable-default-props-03](../../docs/gotchas/react.md#reactshared-mutable-default-props-03--module-level-default-props-are-shared-and-the-builder-mutates-them-in-place) (marker `G-RC05`). +- **`form`/`forms` and `submission`/`submissions` are different Redux modules** (single-entity CRUD vs list+pagination). They're legacy `.js`; TS won't catch a mix-up. See [react/singular-plural-redux-modules-04](../../docs/gotchas/react.md#reactsingular-plural-redux-modules-04--formforms-and-submissionsubmissions-are-different-modules). +- **Keep nothing secret/license-gated in source** and keep `GOTCHA` markers opaque — `src/` is published to the public OSS repo. +- **Match the file's era.** New components/hooks → `.tsx`/`.ts`, function components, hooks. Preserve the legacy `.js` modules (`src/modules/`, `types.js`, `utils.js`) and the deprecated `ReactComponent.jsx` — don't TS-convert them. Adds to [`/STANDARDS.md`](../../STANDARDS.md); never overrides it. + +## Ceiling — emerging patterns + +- **Pattern: a wrapper component holds the instance in `useState`, a `renderElement` ref, and an `isMounted` ref; the create-effect `await`s `.ready`, guards liveness, destroys the previous instance, then `setInstance`. Example:** [`src/components/Form.tsx`](./src/components/Form.tsx) (lines 252–382) — the smallest reference bridge. +- **Pattern: data props are applied to the live instance in their own `isEqual`-guarded effect, never via the create-effect deps. Example:** [`src/components/Form.tsx`](./src/components/Form.tsx) submission-sync effect (lines 373–379). +- **Pattern: a prop that is only ever read or invoked — a callback, or the `handlers` rest-spread — goes through a ref refreshed in a post-commit effect, so its identity cannot drive a resubscribe or a rebuild. Example:** [`src/components/Form.tsx`](./src/components/Form.tsx) `handlersRef` (marker `G-RC01`). +- **Pattern: on redraw, destroy the not-yet-ready previous instance before creating the replacement via a `pendingBuilder` ref. Example:** [`src/components/FormBuilder.tsx`](./src/components/FormBuilder.tsx) (lines 189–209, marker `G-RC02`). + +## Blast radius + +**4 dependents, tier: medium** (`@formio/enterprise-form-builder-react`, `oss-portal`, `react-efb-demo`, `react-playground`). See [`/docs/dependencies/react.md`](../../docs/dependencies/react.md). + +## Test & build + +```sh +pnpm -F @formio/react build # tsc --project tsconfig.json → lib/ +pnpm -F @formio/react test # jest (jsdom + Testing Library) — REAL runner +pnpm -F @formio/react lint # eslint src — REAL +pnpm -F @formio/react check-types # tsc --noEmit +``` + +- **The runner is `jest`, not mocha.** Root CLAUDE.md says "Mocha is primary" — that does **not** apply here; `test`/`lint` are real commands that run real tests (unlike `@formio/angular`/`@formio/bootstrap`, whose scripts are stubs). +- **"Green"** = `test` + `lint` + `check-types`. For any lifecycle change, behavioral green also means exercising the EFB + demo apps (`react-efb-demo`, `react-playground`, `oss-portal` in production mode) — that's where redraw/StrictMode bugs surface. +- Single test file: `cd packages/react && pnpm jest src/components/__tests__/.test.tsx`. + +## Hot paths & gotchas + +See [`/docs/gotchas/react.md`](../../docs/gotchas/react.md). Entries: `react/async-create-race-01`, `react/create-effect-deps-rebuild-02`, `react/shared-mutable-default-props-03`, `react/singular-plural-redux-modules-04`. + +## Cross-cutting triggers + +- **Touching any create/redraw lifecycle** (create-effect, `isMounted`/`pendingBuilder` refs, instance recreation in `Form.tsx`/`FormBuilder.tsx`/`FormEdit.tsx`) → read [`/docs/cross-cutting/framework-wrapper-lifecycle.md`](../../docs/cross-cutting/framework-wrapper-lifecycle.md). This is the `@formio/js ↔ @formio/react` create/redraw contract (FIO-11128, FIO-11245, FIO-9944). +- **Touching `destroy()` / teardown / async continuations** → read [`/docs/cross-cutting/component-lifecycle.md`](../../docs/cross-cutting/component-lifecycle.md). After `destroy(true)`, instance fields are deleted; this is a 6-package contract (FIO-10902). +- **Any rendering/validation/submission behavior question** → it's almost certainly `@formio/js`, not here. Read [`/docs/architecture/formio.js.md`](../../docs/architecture/formio.js.md) and [`/docs/gotchas/formio.js.md`](../../docs/gotchas/formio.js.md) first. +- **Adding a new `@formio/js` import** → use a declared `exports` subpath only (`@formio/js` or `@formio/js/utils`); never `@formio/js/lib/...`. See [`/docs/dependencies/react.md`](../../docs/dependencies/react.md#formiojs-subpath-rule). +- **Changing a `FormBuilder`/`Form` callback signature** (`onSaveComponent`, `onDeleteComponent`, `onChange`, …) → these are a shared contract with `@formio/enterprise-form-builder-react`, which threads them into its conflict-change tracking. FIO-10763 widened `onSaveComponent` (`path`, `isNew`) across both packages + needed two changesets. See the EFB-wrapper-stack row in [`/docs/cross-cutting/README.md`](../../docs/cross-cutting/README.md). +- **`major` bump** → coordinate the OSS release (ships to `github.com/formio/react`); the planned major removes `ReactComponent`. + +## References + +- Repo-wide: [`/CLAUDE.md`](../../CLAUDE.md), [`/STANDARDS.md`](../../STANDARDS.md) +- Architecture: [`/docs/architecture/react.md`](../../docs/architecture/react.md) +- Dependencies: [`/docs/dependencies/react.md`](../../docs/dependencies/react.md) +- Gotchas: [`/docs/gotchas/react.md`](../../docs/gotchas/react.md) +- Renderer it wraps: [`/docs/architecture/formio.js.md`](../../docs/architecture/formio.js.md) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cf7516f8a..a6b16fbe3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -2,10 +2,10 @@ Thank you for helping this project become a better library :) -- [Triage](#triage) -- [Reporting Bugs](#reporting-bugs) -- [Providing a feature request](#providing-a-feature-request) -- [Pull requests](#pull-requests) +- [Triage](#triage) +- [Reporting Bugs](#reporting-bugs) +- [Providing a feature request](#providing-a-feature-request) +- [Pull requests](#pull-requests) ## Triage @@ -23,21 +23,21 @@ Open an issue, making sure to follow the Feature request template. ### Getting started -- If there isn't one already, open an issue describing the bug or feature request that you are going to solve in your pull request. -- Create a fork of react-native-maps - - If you already have a fork, make sure it is up to date -- Git clone your fork and run `yarn` in the base of the cloned repo to setup your local development environment. -- Create a branch from the master branch to start working on your changes. +- If there isn't one already, open an issue describing the bug or feature request that you are going to solve in your pull request. +- Create a fork of react-native-maps + - If you already have a fork, make sure it is up to date +- Git clone your fork and run `yarn` in the base of the cloned repo to setup your local development environment. +- Create a branch from the master branch to start working on your changes. ### Committing -- When you made your changes, run `yarn lint` & `yarn test` to make sure the code you introduced doesn't cause any obvious issues. -- When you are ready to commit your changes, use the [Github conventional commits](https://gist.github.com/qoomon/5dfcdf8eec66a051ecd85625518cfd13) convention for you commit messages, as we use your commits when releasing new versions. -- Use present tense: "add awesome component" not "added awesome component" -- Limit the first line of the commit message to 100 characters -- Reference issues and pull requests before committing +- When you made your changes, run `yarn lint` & `yarn test` to make sure the code you introduced doesn't cause any obvious issues. +- When you are ready to commit your changes, use the [Github conventional commits](https://gist.github.com/qoomon/5dfcdf8eec66a051ecd85625518cfd13) convention for you commit messages, as we use your commits when releasing new versions. +- Use present tense: "add awesome component" not "added awesome component" +- Limit the first line of the commit message to 100 characters +- Reference issues and pull requests before committing ### Creating the pull request -- The title of the PR needs to follow the same conventions as your commit messages, as it might be used in case of a squash merge. -- Create the pull request against the beta branch. +- The title of the PR needs to follow the same conventions as your commit messages, as it might be used in case of a squash merge. +- Create the pull request against the beta branch. diff --git a/README.md b/README.md index e90654887..fbcb3a9fb 100644 --- a/README.md +++ b/README.md @@ -2,8 +2,8 @@ A React library for rendering out forms based on the [Form.io](https://www.form.io) platform. -Official Documentation --------------------------- +## Official Documentation + For the latest documentation, release information, and guides, always refer to the official Form.io Help Documentation available here: **[https://help.form.io](https://help.form.io/dev/javascript-development/frameworks#react)** @@ -324,7 +324,7 @@ A React component wrapper around [a Form.io form](https://help.form.io/dev/form- | `onCustomEvent` | `(event: { type: string; component: Component; data: JSON; event?: Event; }) => void` | | A callback function that is triggered from a button component configured with "Event" type. | | `onPrevPage` | `(page: number, submission: JSON) => void` | | A callback function for Wizard forms that gets called when the "Previous" button is pressed. | | `onNextPage` | `(page: number, submission: JSON) => void` | | A callback function for Wizard forms that gets called when the "Next" button is pressed. | -| `otherEvents` | `[event: string]: (...args: any[]) => void;` | | A "catch-all" prop for subscribing to other events (for a complete list, see [our documentation](https://help.form.io/dev/form-development/form-renderer#form-events)). | +| `otherEvents` | `[event: string]: (...args: any[]) => void;` | | A "catch-all" prop for subscribing to other events (for a complete list, see [our documentation](https://help.form.io/dev/form-development/form-renderer#form-events)). | #### Examples @@ -490,7 +490,7 @@ A React component wrapper around [a Form.io form builder](https://help.form.io/d | Name | Type | Default | Description | | ------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | ------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `initialForm` | `FormType` | | The JSON form definition of the initial form to be rendered in the builder. Oftentimes, this must be a stable reference; otherwise it may destroy and recreate the underlying builder instance and cause unexpected behavior. | -| `options` | `FormBuilderOptions` | | The form builder options. See [here](https://help.form.io/dev/form-development/form-builder#form-builder-options) for more details. | +| `options` | `FormBuilderOptions` | | The form builder options. See [here](https://help.form.io/dev/form-development/form-builder#form-builder-options) for more details. | | `onBuilderReady` | `(instance: FormBuilder) => void` | | A callback function that gets called when the form builder has rendered. It is useful for accessing the underlying @formio/js FormBuilder instance. | | `onChange` | `(form: FormType) => void` | | A callback function that gets called when the form being built has changed. | | `onSaveComponent` | `(component: Component, original: Component, parent: Component, path: string, index: number, isNew: boolean, originalComponentSchema: Component) => void;` | | A callback function that gets called when a component is saved in the builder. | @@ -856,7 +856,8 @@ const FormActionButton: FormGridComponentProps['FormActionButton'] = ({ > {action && action.name === 'Edit' ? 'Edit' : ''} + >{' '} + {action && action.name === 'Edit' ? 'Edit' : ''} ); diff --git a/jest.config.js b/jest.config.js index 4f1a6811d..27d133857 100644 --- a/jest.config.js +++ b/jest.config.js @@ -11,6 +11,7 @@ module.exports = { ], transform: { '^.+\\.css$': 'jest-transform-css', + '^.+\\.jsx$': 'ts-jest', }, transformIgnorePatterns: [], verbose: true, diff --git a/package.json b/package.json index 47a8cb39d..3b85d3a90 100644 --- a/package.json +++ b/package.json @@ -1,12 +1,14 @@ { "name": "@formio/react", - "version": "6.2.1", + "version": "6.2.2", "description": "React renderer for form.io forms.", "main": "lib/index.js", "scripts": { "test": "jest", "build": "tsc --project tsconfig.json", - "lint": "eslint src" + "lint": "eslint src", + "check-types": "tsc --noEmit", + "lint:fix": "eslint src --fix" }, "repository": { "type": "git", @@ -32,8 +34,8 @@ "prop-types": "^15.8.1" }, "devDependencies": { - "@formio/core": "^2.7.0", - "@formio/js": "^5.4.0", + "@formio/core": "^2.9.0", + "@formio/js": "^5.7.0", "@testing-library/dom": "^10.4.0", "@testing-library/jest-dom": "^6.6.2", "@testing-library/react": "^16.1.1", diff --git a/src/components/Form.tsx b/src/components/Form.tsx index 2520b3f51..10384aa35 100644 --- a/src/components/Form.tsx +++ b/src/components/Form.tsx @@ -282,7 +282,7 @@ export const Form = (props: FormProps) => { isMounted.current = true; return () => { isMounted.current = false; - } + }; }, []); useEffect(() => { @@ -316,6 +316,7 @@ export const Form = (props: FormProps) => { ); if (instance) { + // GOTCHA(G-RC01) if (!isMounted.current) { instance.destroy(true); return; @@ -345,22 +346,29 @@ export const Form = (props: FormProps) => { }; createInstance(); + // GOTCHA(G-RC03) }, [formConstructor, formReadyCallback, formSource, options, url]); + const handlersRef = useRef(handlers); + useEffect(() => { + handlersRef.current = handlers; + }); + useEffect(() => { - let onAnyHandler = null; - if (formInstance && Object.keys(handlers).length > 0) { - onAnyHandler = (...args: [string, ...any[]]) => - onAnyEvent(handlers, ...args); - formInstance.onAny(onAnyHandler); + if (!formInstance) { + return; } + // GOTCHA(G-RC01) + const events = formInstance.events; + const onAnyHandler = (...args: [string, ...any[]]) => + onAnyEvent(handlersRef.current, ...args); + formInstance.onAny(onAnyHandler); + return () => { - if (formInstance && onAnyHandler) { - formInstance.offAny(onAnyHandler); - } + events?.offAny(onAnyHandler); }; - }, [formInstance, handlers]); + }, [formInstance]); useEffect(() => { if (formInstance && submission) { diff --git a/src/components/FormBuilder.tsx b/src/components/FormBuilder.tsx index 4281b9b26..1138de380 100644 --- a/src/components/FormBuilder.tsx +++ b/src/components/FormBuilder.tsx @@ -24,7 +24,7 @@ export type FormBuilderProps = { index: number, originalComponentSchema: Component, path: string, - isNew: boolean + isNew: boolean, ) => void; onAddComponent?: ( component: Component, @@ -72,7 +72,7 @@ const toggleEventHandlers = ( index, originalComponentSchema, path, - isNew + isNew, ); onChange?.(structuredClone(builder.instance?.form)); }, @@ -121,7 +121,10 @@ const createBuilderInstance = async ( formSource: FormSource | undefined, element: HTMLDivElement, options: FormBuilderProps['options'] = {}, - setBuiderStatus: (builder: FormioFormBuilder, ready: boolean) => void = () => {} + setBuiderStatus: ( + builder: FormioFormBuilder, + ready: boolean, + ) => void = () => {}, ): Promise => { const builder = BuilderConstructor ? new BuilderConstructor(element, formSource, options) @@ -183,8 +186,9 @@ export const FormBuilder = ({ initialForm && typeof initialForm !== 'string' ? structuredClone(initialForm) : null; - // destroy prev builder that is not ready before to create the new one - if (pendingBuilder.current) { + // GOTCHA(G-RC02) + // destroy prev builder that is not ready before to create the new one + if (pendingBuilder.current) { const prevBuilder = pendingBuilder.current; // wait the prev builder to be ready before destroying it await prevBuilder.ready; @@ -192,8 +196,8 @@ export const FormBuilder = ({ prevBuilder.instance?.destroy(true); prevBuilder.destroy(true); pendingBuilder.current = null; - } - + } + const builder = await createBuilderInstance( Builder, currentFormSourceJsonProp.current || initialForm, @@ -201,7 +205,7 @@ export const FormBuilder = ({ options, (builder, ready) => { pendingBuilder.current = ready ? null : builder; - } + }, ); if (builder) { @@ -215,7 +219,7 @@ export const FormBuilder = ({ } setBuilderInstance((prevInstance) => { if (prevInstance) { - prevInstance.instance?.destroy(true); + prevInstance.instance?.destroy(true); prevInstance.destroy(true); } return builder; @@ -226,6 +230,7 @@ export const FormBuilder = ({ }; createInstance(); + // GOTCHA(G-RC04) }, [Builder, initialForm, onBuilderReady, options]); useEffect(() => { diff --git a/src/components/FormEdit.tsx b/src/components/FormEdit.tsx index 84e8f016a..52e3b7bb5 100644 --- a/src/components/FormEdit.tsx +++ b/src/components/FormEdit.tsx @@ -193,6 +193,7 @@ const DEFAULT_SETTINGS_FORM_OPTIONS = {}; const DEFAULT_COMPONENTS = {}; export const FormEdit = ({ + // GOTCHA(G-RC05) initialForm = fastCloneDeep(DEFAULT_INITAL_FORM), settingsForm = fastCloneDeep(DEFAULT_SETTINGS_FORM), settingsFormOptions = fastCloneDeep(DEFAULT_SETTINGS_FORM_OPTIONS), @@ -227,11 +228,11 @@ export const FormEdit = ({ const builderRef = useRef(null); const handleSaveForm = async () => { - const currentForm = builderRef.current?.form as FormType; - if (!currentForm) { - console.warn("Could not find current form when trying to save"); - return; - } + const currentForm = builderRef.current?.form as FormType; + if (!currentForm) { + console.warn('Could not find current form when trying to save'); + return; + } const formToSave: FormType = { ...currentForm, ...settingsFormData.current, diff --git a/src/components/Report.jsx b/src/components/Report.jsx index a170849a3..85fc33caf 100644 --- a/src/components/Report.jsx +++ b/src/components/Report.jsx @@ -35,25 +35,48 @@ export const Report = (props) => { return createPromise; }; + const propsRef = useRef(props); + useEffect(() => { + propsRef.current = props; + }); + + const subscriptionRef = useRef(null); + + const unsubscribe = () => { + if (!subscriptionRef.current) { + return; + } + const { events, handler } = subscriptionRef.current; + events?.offAny(handler); + subscriptionRef.current = null; + }; + const onAnyEvent = (event, ...args) => { if (event.startsWith('formio.')) { const funcName = `on${event.charAt(7).toUpperCase()}${event.slice(8)}`; + const handlers = propsRef.current; // eslint-disable-next-line no-prototype-builtins if ( - props.hasOwnProperty(funcName) && - typeof props[funcName] === 'function' + handlers.hasOwnProperty(funcName) && + typeof handlers[funcName] === 'function' ) { - props[funcName](...args); + handlers[funcName](...args); } } }; const initializeFormio = () => { - if (createPromise) { - instance.onAny(onAnyEvent); + if (!createPromise) { + return; } + unsubscribe(); + const events = instance.events; + instance.onAny(onAnyEvent); + subscriptionRef.current = { events, handler: onAnyEvent }; }; + useEffect(() => unsubscribe, []); + useEffect(() => { const { src } = props; if (src) { diff --git a/src/components/__tests__/Form.test.tsx b/src/components/__tests__/Form.test.tsx index 20f00a74e..6a0ce7628 100644 --- a/src/components/__tests__/Form.test.tsx +++ b/src/components/__tests__/Form.test.tsx @@ -1,8 +1,9 @@ -import { render } from '@testing-library/react'; +import { act, render, waitFor } from '@testing-library/react'; import { screen } from '@testing-library/dom'; import '@testing-library/jest-dom'; +import { EventEmitter, Webform } from '@formio/js'; -import { Form } from '../Form'; +import { Form, FormOptions, FormProps } from '../Form'; const simpleForm = { display: 'form' as const, @@ -41,3 +42,442 @@ test('loads and displays a simple form', async () => { }; render(); }); + +test('keeps exactly one `any` listener and detaches it on unmount', async () => { + // The emitter is what leaks: `createWebformInstance` stores it on the + // caller's `options` object, so it outlives any one form instance. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + const onChange = jest.fn(); + + const { rerender, unmount } = render( + , + ); + await waitFor(() => expect(events.listenerCount('any')).toBe(1)); + + // A new handler identity must not churn or duplicate the subscription. + rerender( + onChange()} />, + ); + expect(events.listenerCount('any')).toBe(1); + + unmount(); + await waitFor(() => expect(events.listenerCount('any')).toBe(0)); +}); + +// A second, structurally different form, so swapping `src` genuinely rebuilds +// the instance instead of tripping the deep-equal early return in the create +// effect. +const otherForm = { + display: 'form' as const, + components: [ + { + label: 'Email', + key: 'email', + type: 'email', + input: true, + }, + ], +}; + +// `Element.d.ts` declares `events` as non-optional, but `destroy(true)` deletes +// it, so the type cannot express the state this asserts on. Read it through a +// view that admits `undefined`. (Same mismatch is why `events?.offAny(...)` in +// `Form.tsx` is load-bearing rather than dead defensiveness.) +const eventsOf = (instance: Webform) => + (instance as unknown as { events?: EventEmitter }).events; + +// `Webform['submission']` is typed as a bare `object`; narrow it for assertions. +const submissionOf = (instance: Webform) => + instance.submission as { data: Record }; + +// `createWebformInstance` only ever does `new FormClass(element, source, options)` +// and awaits `.ready`, so a stand-in needs nothing more than that. Deferring the +// resolution is what lets a test place two creates in one React batch, or hand +// back an instance that is already torn down. +type FakeInstance = { + events?: EventEmitter; + form?: unknown; + src?: unknown; + destroy: jest.Mock; + onAny: jest.Mock; + offAny: jest.Mock; +}; + +const deferredFormClass = () => { + const created: FakeInstance[] = []; + const pending: Array<{ + resolve: (value: unknown) => void; + instance: FakeInstance; + }> = []; + + class DeferredForm { + ready: Promise; + + constructor( + _element: HTMLElement, + _source: unknown, + options: FormOptions, + ) { + // Route through `instance.events`, never a captured emitter: `Element` + // reads `this.events` on every call and short-circuits once + // `teardown()` has deleted it, and a fake that stays subscribed after + // destroy would hide exactly the bug these tests exist for. + const instance: FakeInstance = { + events: options.events, + destroy: jest.fn(() => delete instance.events), + onAny: jest.fn((cb: (...args: unknown[]) => void) => + instance.events?.onAny(cb), + ), + offAny: jest.fn((cb: (...args: unknown[]) => void) => + instance.events?.offAny(cb), + ), + }; + created.push(instance); + this.ready = new Promise((resolve) => + pending.push({ resolve, instance }), + ); + } + } + + return { + FormClass: DeferredForm as unknown as FormProps['FormClass'], + created, + // Settle every outstanding create at once, so their continuations land in + // a single React batch. `as` rewrites what each one resolves to. + settle: (as?: (instance: FakeInstance) => unknown) => + pending + .splice(0) + .forEach(({ resolve, instance }) => + resolve(as ? as(instance) : instance), + ), + }; +}; + +test('rewires handlers and submission onto the instance a rerender builds', async () => { + // FIO-9589 (formio/react#595): the handler and submission effects key on + // `formInstance`. They used to key on an `instanceIsReady` boolean that only + // ever went true, so the second instance a rerender built came up with no + // handlers and no submission attached. Each instance gets its own emitter + // here (no `options` passed), so firing through the new instance proves the + // new instance is the one that is wired. + const instances: Webform[] = []; + // Stable identity: `onFormReady` is a create-effect dependency, and a fresh + // closure each render would rebuild the form on every render. + const captureInstance = (instance: Webform) => { + instances.push(instance); + }; + const onChange = jest.fn(); + + const { rerender } = render( + , + ); + await waitFor(() => expect(instances).toHaveLength(1)); + + rerender( + , + ); + await waitFor(() => expect(instances).toHaveLength(2)); + + await waitFor(() => + expect(submissionOf(instances[1]).data).toMatchObject({ + email: 'joe@example.com', + }), + ); + + onChange.mockClear(); + instances[1].emit('change', { data: { email: 'joe@example.com' } }); + expect(onChange).toHaveBeenCalledTimes(1); +}); + +test('destroys the instance a rerender replaces', async () => { + // FIO-9944 (formio/react#610): the replacement is set through a state updater + // that destroys the instance it displaces, so a rebuilt form cannot strand + // the old one. `destroy(true)` runs `teardown()`, which deletes `events` -- + // that deletion is the observable proof it ran. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + const instances: Webform[] = []; + const captureInstance = (instance: Webform) => { + instances.push(instance); + }; + + const { rerender } = render( + , + ); + await waitFor(() => expect(instances).toHaveLength(1)); + + rerender( + , + ); + await waitFor(() => expect(instances).toHaveLength(2)); + + expect(instances[0]).not.toBe(instances[1]); + await waitFor(() => expect(eventsOf(instances[0])).toBeUndefined()); + // The displaced instance took its subscription with it: the shared emitter is + // left holding exactly the replacement's listener. + await waitFor(() => expect(events.listenerCount('any')).toBe(1)); +}); + +test('destroys an instance whose promise resolves after unmount', async () => { + // FIO-9944 / GOTCHA(G-RC01): `createWebformInstance` is awaited, so the + // instance can arrive after React has unmounted the component. Nothing else + // can reach it -- `formInstance` state was never set, so the destroy effect + // has nothing to clean up -- and without the `isMounted` guard it is + // stranded, fully rendered, for the life of the page. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + const destroy = jest.spyOn(Webform.prototype, 'destroy'); + + try { + const { unmount } = render(); + unmount(); + + await waitFor(() => expect(destroy).toHaveBeenCalledWith(true)); + expect(events.listenerCount('any')).toBe(0); + } finally { + destroy.mockRestore(); + } +}); + +test('dispatches to the latest handler without rebuilding the subscription', async () => { + // The handlers are read through a ref, so a new handler identity must reach + // the existing listener rather than tear it down and resubscribe. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + const instances: Webform[] = []; + const captureInstance = (instance: Webform) => { + instances.push(instance); + }; + const first = jest.fn(); + const second = jest.fn(); + + const { rerender } = render( + , + ); + await waitFor(() => expect(instances).toHaveLength(1)); + const subscription = events.listeners('any')[0]; + + rerender( + , + ); + + expect(instances).toHaveLength(1); + expect(events.listenerCount('any')).toBe(1); + // Identity, not just count: unsubscribing and resubscribing within one commit + // also leaves exactly one listener and also dispatches to the latest handler, + // so a count alone cannot tell the two apart. + expect(events.listeners('any')[0]).toBe(subscription); + + first.mockClear(); + second.mockClear(); + instances[0].emit('change', { data: {} }); + expect(second).toHaveBeenCalledTimes(1); + expect(first).not.toHaveBeenCalled(); +}); + +test('destroys an instance that is superseded before it ever commits', async () => { + // FIO-9944 (formio/react#610), the case the destroy effect cannot catch. Two + // creates can resolve in one React batch; React then commits only the last, + // so the destroy effect -- which keys on the COMMITTED `formInstance` -- never + // sees the first one and it is stranded. The state updater destroys whatever + // it displaces, committed or not. Resolving both `ready` promises by hand is + // what makes the batch deterministic rather than a timing race. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + const { FormClass, created, settle } = deferredFormClass(); + + const { rerender } = render( + , + ); + rerender(); + expect(created).toHaveLength(2); + + // Both continuations land before React re-renders, so both instances are set + // in a single batch and only the second one is ever committed. + await act(async () => settle()); + + expect(created[0].destroy).toHaveBeenCalledWith(true); + expect(created[1].destroy).not.toHaveBeenCalled(); +}); + +// --- error and edge paths ------------------------------------------------- +// Everything above exercises a form that builds. These cover the paths where it +// does not, or where the instance the effects are handed is not the healthy +// object they expect. The subscription must come out of each with the emitter +// holding exactly the listeners that belong to a live, mounted form. + +test('leaves the previous instance subscribed when a rebuild produces nothing', async () => { + // `createWebformInstance` resolving falsy is the "form failed to build" path: + // a bad `src`, a constructor that bails. `setFormInstance` is never reached, + // so the committed instance stays committed -- and its subscription has to + // survive intact, because that form is still the one on screen. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + const { FormClass, created, settle } = deferredFormClass(); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + try { + const { rerender } = render( + , + ); + await act(async () => settle()); + await waitFor(() => expect(events.listenerCount('any')).toBe(1)); + + rerender( + , + ); + await act(async () => settle(() => null)); + + expect(created).toHaveLength(2); + expect(warn).toHaveBeenCalledWith('Failed to create form instance'); + // The surviving instance keeps its listener, and the stillborn one adds none. + expect(events.listenerCount('any')).toBe(1); + expect(created[0].destroy).not.toHaveBeenCalled(); + expect(created[0].offAny).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } +}); + +test('does not subscribe to an instance that arrives already torn down', async () => { + // The subscription effect reads `formInstance.events`, and `destroy(true)` + // deletes it. An instance can reach the effect in that state, so both the + // subscribe and the cleanup have to tolerate a missing emitter rather than + // throw on the way through. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + const { FormClass, created, settle } = deferredFormClass(); + + const { unmount } = render( + , + ); + await act(async () => + settle((instance) => { + delete instance.events; + return instance; + }), + ); + + expect(created).toHaveLength(1); + expect(events.listenerCount('any')).toBe(0); + expect(() => unmount()).not.toThrow(); + expect(events.listenerCount('any')).toBe(0); +}); + +test('leaves a sibling form subscribed when one of them unmounts', async () => { + // Two forms handed the same `options` share one emitter, so the cleanup has + // to remove the unmounting form's listener and only that one. Removing the + // wrong listener -- or, before FIO-12286, none at all -- is invisible until + // the surviving form stops receiving events or starts double-firing. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + const instances: Webform[] = []; + const captureInstance = (instance: Webform) => { + instances.push(instance); + }; + const closing = jest.fn(); + const staying = jest.fn(); + + const Pair = ({ showClosing }: { showClosing: boolean }) => ( + <> + {showClosing && ( + + )} + + + ); + + const { rerender } = render(); + await waitFor(() => expect(events.listenerCount('any')).toBe(2)); + + rerender(); + await waitFor(() => expect(events.listenerCount('any')).toBe(1)); + + closing.mockClear(); + staying.mockClear(); + events.emit('formio.change', { data: {} }); + expect(staying).toHaveBeenCalledTimes(1); + expect(closing).not.toHaveBeenCalled(); +}); + +test('returns the shared emitter to zero listeners across mount/unmount churn', async () => { + // The reported symptom was a count that climbed 1 -> 2 -> 3 over a session as + // forms came and went against one long-lived emitter. One cycle can pass on a + // cleanup that only half works; a run of them cannot. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + const onChange = jest.fn(); + + for (let cycle = 0; cycle < 5; cycle++) { + const { unmount } = render( + , + ); + await waitFor(() => expect(events.listenerCount('any')).toBe(1)); + unmount(); + await waitFor(() => expect(events.listenerCount('any')).toBe(0)); + } +}); + +test('adds no listener when the component unmounts mid-create, repeatedly', async () => { + // The unmount race, run in a loop against one emitter: each aborted create is + // destroyed by the `isMounted` guard before it can subscribe, so nothing + // accumulates even when no form ever finishes building. + const events = new EventEmitter({ wildcard: false, maxListeners: 0 }); + const options = { events }; + + for (let cycle = 0; cycle < 5; cycle++) { + const { unmount } = render( + , + ); + unmount(); + } + + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + expect(events.listenerCount('any')).toBe(0); +}); diff --git a/src/components/__tests__/Report.test.tsx b/src/components/__tests__/Report.test.tsx new file mode 100644 index 000000000..2ede77c50 --- /dev/null +++ b/src/components/__tests__/Report.test.tsx @@ -0,0 +1,103 @@ +import { render, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { EventEmitter, Formio } from '@formio/js'; + +type ReportOptions = { events?: EventEmitter }; + +// `Formio.Report` ships in the reporting module, which this package does not +// depend on, and `Report.jsx` captures it at module load. The stand-in mirrors +// the two things the component relies on: `Form` writes its emitter onto the +// caller's `options` (`Form.js`, `this.options.events = this.events`), so the +// emitter outlives any one instance, and `ready` resolves to the INNER form, +// not the wrapper the `any` listener is attached to. +class FakeReport { + events?: EventEmitter; + ready: Promise<{ destroy: jest.Mock; form?: unknown }>; + + constructor( + _element: HTMLElement, + _srcOrReport: unknown, + options: ReportOptions, + ) { + this.events = + options.events || + new EventEmitter({ wildcard: false, maxListeners: 0 }); + options.events = this.events; + this.ready = Promise.resolve({ destroy: jest.fn() }); + } + + onAny(cb: (...args: unknown[]) => void) { + return this.events?.onAny(cb); + } + + offAny(cb: (...args: unknown[]) => void) { + return this.events?.offAny(cb); + } +} + +(Formio as unknown as { Report: unknown }).Report = FakeReport; + +const { Report } = jest.requireActual<{ + Report: (props: Record) => JSX.Element; +}>('../Report'); + +const newEmitter = () => new EventEmitter({ wildcard: false, maxListeners: 0 }); + +test('detaches the report `any` listener on unmount', async () => { + const events = newEmitter(); + const options = { events }; + + const { unmount } = render( + , + ); + await waitFor(() => expect(events.listenerCount('any')).toBe(1)); + + unmount(); + expect(events.listenerCount('any')).toBe(0); +}); + +test('keeps exactly one `any` listener when the report is rebuilt', async () => { + const events = newEmitter(); + const options = { events }; + + const { rerender } = render( + , + ); + await waitFor(() => expect(events.listenerCount('any')).toBe(1)); + + rerender(); + await waitFor(() => expect(events.listenerCount('any')).toBe(1)); +}); + +test('dispatches to the latest handler props without a rebuild', async () => { + const events = newEmitter(); + const options = { events }; + const first = jest.fn(); + const second = jest.fn(); + + const { rerender } = render( + , + ); + await waitFor(() => expect(events.listenerCount('any')).toBe(1)); + const subscription = events.listeners('any')[0]; + + rerender( + , + ); + + // Identity, not just count: an unsubscribe/resubscribe would also dispatch to + // the latest props, so a count alone cannot tell the two apart. + expect(events.listeners('any')[0]).toBe(subscription); + + events.emit('formio.rowClick', { row: 1 }); + expect(second).toHaveBeenCalledWith({ row: 1 }); + expect(first).not.toHaveBeenCalled(); +}); diff --git a/tsconfig.json b/tsconfig.json index 293a73e00..c7904fd56 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -10,7 +10,7 @@ "baseUrl": "src", "strict": true, "skipLibCheck": true, - "esModuleInterop": true, + "esModuleInterop": true }, "include": ["src/**/*"], "types": [ @@ -18,7 +18,7 @@ "jest", "@testing-library/jest-dom", "react", - "react-dom", + "react-dom" ], - "exclude": ["node_modules", "lib", "test"], + "exclude": ["node_modules", "lib", "test"] }