diff --git a/CLAUDE.md b/CLAUDE.md index 385b34fa..067acde7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -132,11 +132,19 @@ resumes. Two things pause it: - react-final-form, for the first render; - the brackets an engine node opens in a layout effect and closes in a passive effect, on mount and when its `showIf` flips (`registrationsOf`, used by `RenderComponent` in `mapper.tsx`). Fields register in passive effects, - in between. + in between. `Modal` brackets its popup's fields the same way as it opens and closes, since nothing mounts when a + popup closes. A field that goes while validation is paused tells nothing until it resumes. React 16 runs a removed component's -effect cleanups before any layout effect, so there a field that goes is not bracketed. `form-mount-budget.test.js` -and `later-mount-budget.test.js` count what the bursts cost. +effect cleanups before any layout effect, so there a field that goes is not bracketed. + +react-final-form also probes each field in its first render, registering and unregistering it for its first +state. While validation runs, the probe registers without its validator, as it would on a document's first render: +with one, it validated every field in the form, once per new field. The field gets its error when its node's +bracket closes. That depends on its first registration from an effect coming inside a bracket, since +react-final-form takes the first notification after it as the probe's state. + +`form-mount-budget.test.js` and `later-mount-budget.test.js` count what the bursts cost. To find what a cost comes from, count before naming the cause: wrap the suspected function or component with `jest.mock` and count calls per action. Comparing timings with and without a feature does not name a cause. diff --git a/src/core/engine/components/Modal.tsx b/src/core/engine/components/Modal.tsx index b03e3e6b..2da6ae38 100644 --- a/src/core/engine/components/Modal.tsx +++ b/src/core/engine/components/Modal.tsx @@ -1,5 +1,6 @@ -import React, { useContext, useEffect, useRef, useState } from 'react' +import React, { useContext, useEffect, useLayoutEffect, useRef, useState } from 'react' import { createPortal } from 'react-dom' +import { registrationsOf } from '../../modules/form/batchedForm' import Button from '../../components/Button' import Text from '../../components/Text' import View from '../../components/View' @@ -8,6 +9,10 @@ import { _ } from '../../utils/translations' import { AppContext } from '../../contexts' import type { Translate } from '../../utils/_envs' +// A layout effect in a browser. On the server, where no effect runs, an effect: React 16 and 17 warn about a layout +// effect there. +const useBeforePaintEffect = typeof window === 'undefined' ? useEffect : useLayoutEffect + localiseTranslation({ CANCEL: { [l.ENGLISH]: 'Cancel', @@ -70,7 +75,7 @@ let popups = 0 * of the document constructed LAST: on a page with two documents, the first one's popup showed * the second one's language around its own content. */ -const Modal = ({ translate }: { translate?: Translate }) => { +const Modal = ({ translate, form }: { translate?: Translate, form?: unknown }) => { const popup = useContext(AppContext) const { isOpen, title, content, togglePopupState } = popup const activeClass = isOpen ? ' active' : '' @@ -95,6 +100,12 @@ const Modal = ({ translate }: { translate?: Translate }) => { } }, [isOpen]) + // A popup's fields go when it closes, and nothing mounts then whose node would bracket their going: the + // dialog brackets what opening and closing it registers and unregisters, in `form`, the document's, which + // its content's fields register with (`registrationsOf`, `modules/form/batchedForm.ts`). + useBeforePaintEffect(() => { registrationsOf(form)?.begin() }, [form, isOpen]) + useEffect(() => { registrationsOf(form)?.end() }, [form, isOpen]) + const onKeyDown = (event: React.KeyboardEvent) => { // A control inside that used the key claims it: an open listbox closes on Escape and says so. if (event.key !== 'Escape' || event.defaultPrevented) return diff --git a/src/core/engine/rules.tsx b/src/core/engine/rules.tsx index 5e3a0912..05e28288 100644 --- a/src/core/engine/rules.tsx +++ b/src/core/engine/rules.tsx @@ -458,7 +458,7 @@ export class UIRender extends DocumentInstance { document with no content, and a host relies on Enter in a field submitting nothing. */} {content} {childAfter} - + // The configuration props are published here, around the whole subtree, rather than diff --git a/src/core/modules/form/__tests__/batchedForm.document.test.js b/src/core/modules/form/__tests__/batchedForm.document.test.js index 5d4a6b3a..fa66f400 100644 --- a/src/core/modules/form/__tests__/batchedForm.document.test.js +++ b/src/core/modules/form/__tests__/batchedForm.document.test.js @@ -88,6 +88,29 @@ describe('a document whose fields registered silently as it mounted', () => { expect(heard[heard.length - 1]).toBe(true) }) + it('tells them it is valid when a popup that holds an empty required field closes', async () => { + const meta = { + view: 'Col', + items: [ + { view: 'Input', name: 'title', label: 'Title' }, + { view: 'Button', children: 'Open details', onClick: { name: 'popupOpen', args: ['details'] } }, + { view: 'Popup', id: 'details', title: 'Details', items: [{ view: 'Input', name: 'detail', label: 'Detail', validate: 'required' }] }, + ], + } + render( {}} />) + const { form, heard } = formHolding('title') + expect(heard).toEqual([true]) + + await act(async () => { fireEvent.click(screen.getByRole('button', { name: 'Open details' })) }) + expect(screen.getByLabelText('Detail')).toBeTruthy() + expect(heard[heard.length - 1]).toBe(false) + + await act(async () => { fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' }) }) + expect(screen.queryByRole('dialog')).toBeNull() + expect(form.getState().valid).toBe(true) + expect(heard[heard.length - 1]).toBe(true) + }) + it('tells them so when the row with the empty required field is removed', () => { jest.useFakeTimers() try { diff --git a/src/core/modules/form/__tests__/batchedForm.test.js b/src/core/modules/form/__tests__/batchedForm.test.js index 3fb1602c..cb2f4e6d 100644 --- a/src/core/modules/form/__tests__/batchedForm.test.js +++ b/src/core/modules/form/__tests__/batchedForm.test.js @@ -134,6 +134,25 @@ describe('createBatchedForm', () => { expect(values[values.length - 1]).toEqual({}) }) + it('registers a probe without its validator while validation runs, so that it validates nothing', () => { + const { form } = setUp({ a: '', b: '' }) + const validated = { count: 0 } + const counted = () => value => { + validated.count += 1 + return value ? undefined : 'Required' + } + form.registerField('a', () => undefined, { error: true }, { getValidator: counted }) + validated.count = 0 + const heard = [] + // react-final-form's probe in a field's first render: registered silently, and unregistered at once. + form.registerField('b', state => { heard.push(state) }, { error: true, valid: true }, { silent: true, getValidator: counted })() + expect(validated.count).toBe(0) + expect(heard).toEqual([expect.objectContaining({ error: undefined, valid: true })]) + // The field's own registration validates, and finds the error. + form.registerField('b', () => undefined, { error: true }, { getValidator: counted }) + expect(form.getFieldState('b').error).toBe('Required') + }) + it('leaves a registration that asks to be silent as it is, and its unregistering', () => { const { form } = setUp({ a: 1 }) const batches = { count: 0 } diff --git a/src/core/modules/form/batchedForm.ts b/src/core/modules/form/batchedForm.ts index 266938b0..4ffd245e 100644 --- a/src/core/modules/form/batchedForm.ts +++ b/src/core/modules/form/batchedForm.ts @@ -43,8 +43,9 @@ export function registrationsOf (form: unknown): Registrations | undefined { * one, the form's listeners are told when it was the last field of its name. While validation is paused, that is * held until it resumes, and done once for all the fields that went. * - * A registration that asks to be silent itself, as react-final-form's probe in a field's first render does, is - * left as it is. So is every registration made while validation runs. + * A registration that asks to be silent itself, as react-final-form's probe in a field's first render does, keeps + * its `silent`, and while validation runs, it registers without its validator (see below). Every other registration + * made while validation runs is left as it is. */ export function createBatchedForm> (config: Config): FormApi { const form = createForm(config) @@ -75,7 +76,21 @@ export function createBatchedForm> (config: Con setConfig(name, value) } form.registerField = (name, subscriber, subscription, fieldConfig) => { - if (!form.isValidationPaused() || fieldConfig?.silent) return registerField(name, subscriber, subscription, fieldConfig) + if (fieldConfig?.silent) { + // react-final-form's probe in a field's first render, registered and unregistered at once for the state the + // field first renders with. With a validator and validation running, final-form validated every field for + // it: a burst of n fields mounting beside m others ran about n·m validators before any bracket opened, since + // the probe runs in render. Without one, the field first renders with no error yet, as every field does on a + // document's first render, where react-final-form has validation paused. Its error comes when its node's + // bracket closes. That bracket matters: react-final-form takes the first notification after the field's + // registration from its effect as the state the probe gave it, so that registration must not validate. + // Inside a bracket it registers silently, and the error comes in a later notification. Without the + // brackets, a field the user touched came back from a tab switch without its error, as the tab-switch test + // of `UIRender.form-flows.test.js` shows. + if (form.isValidationPaused()) return registerField(name, subscriber, subscription, fieldConfig) + return registerField(name, subscriber, subscription, {...fieldConfig, getValidator: undefined}) + } + if (!form.isValidationPaused()) return registerField(name, subscriber, subscription, fieldConfig) unheard = true const unregister = registerField(name, subscriber, subscription, {...fieldConfig, silent: true}) return () => { diff --git a/src/demo/markdowns/changelog.md b/src/demo/markdowns/changelog.md index 0d562f6c..cfdb9fc2 100644 --- a/src/demo/markdowns/changelog.md +++ b/src/demo/markdowns/changelog.md @@ -866,6 +866,12 @@ Nothing to do for these: #### Fixes +- **Fields that mount or go later cost the form one pass in two more cases.** react-final-form's probe of a + field's first render validated every field in the form while validation ran, once for each new field: in + Chromium, showing a 200-row table beside 200 required fields took 153 ms, and takes 98. A popup's fields + went one by one when it closed, each validating every field left, since nothing mounts then whose node + brackets them: the dialog brackets them now, and closing a popup that holds such a table takes 27 ms + instead of 447. On React 16 a popup's fields still go one by one. - **A number shown with decimals is formatted by a formatter made once.** `renderFloat`, which the `Currency`, `Float`, `Double5` and `Percent` renderers and `Counter` show numbers through, formatted each fraction with `toLocaleString` and its options: a new `Intl.NumberFormat` for every value, 9.65 µs diff --git a/src/library/__tests__/later-mount-budget.test.js b/src/library/__tests__/later-mount-budget.test.js index b6d10dc5..47bf67ea 100644 --- a/src/library/__tests__/later-mount-budget.test.js +++ b/src/library/__tests__/later-mount-budget.test.js @@ -13,9 +13,13 @@ * - a tab switch to the table computed 21008 field states and ran 5252 validators, and one away from it 20200 and * 5050; * - a `showIf` that showed it computed 21524 and ran 5252, and one that hid it 21322 and 5151. - * Now a switch to it computes 12 states per field, its own two as it registers and the rest in the passes when the - * bracket closes, and validates each field twice; a switch away has no field left to tell. Hiding it validates each - * field once, from the `form.change` that hides it: a field changed with no `validateFields` validates them all. + * Now a switch to it computes 11 states per field, and validates each field once, when the bracket closes; a switch + * away has no field left to tell. Hiding it validates each field once, from the `form.change` that hides it: a field + * changed with no `validateFields` validates them all. + * + * Until 2026-10-14, react-final-form's probe in each field's first render validated every field registered then, + * before any bracket opened. Here that was each field once more. Beside 100 other required fields, it was 101 times + * each new field: showing the table there ran 10502 validators, 10201 of them from the probes. * * React 16 runs a removed component's effect cleanups during the commit, before any layout effect, so no bracket is * open when the table's fields go: there they still go one by one, and cost what they did. A number here that falls @@ -87,7 +91,7 @@ describe('fields that mount after the first render: a 50-row table of two fields const fields = formHolding('rows').getRegisteredFields() const away = await costOf(() => fireEvent.click(screen.getByRole('tab', { name: 'A' }))) expect(fields).toHaveLength(2 * ROWS + 1) - expect(to).toEqual({ computed: 12 * fields.length, validated: 2 * fields.length }) + expect(to).toEqual({ computed: 11 * fields.length, validated: fields.length }) expect(away).toEqual(REMOVALS_BRACKETED ? { computed: 0, validated: 0 } : { computed: 20200, validated: 5050 }) expect(screen.queryByRole('textbox', { name: /Note/ })).toBeNull() }) @@ -100,8 +104,45 @@ describe('fields that mount after the first render: a 50-row table of two fields const fields = form.getRegisteredFields() const hidden = await costOf(() => form.change('show', 'no')) expect(fields).toHaveLength(2 * ROWS + 2) - expect(shown).toEqual({ computed: 1431, validated: 2 * (fields.length - 1) }) + expect(shown).toEqual({ computed: 1330, validated: fields.length - 1 }) expect(hidden).toEqual(REMOVALS_BRACKETED ? { computed: 822, validated: fields.length - 1 } : { computed: 21322, validated: 5151 }) expect(form.getRegisteredFields()).toEqual(['show']) }) + + it('costs a `showIf` that shows it beside 100 other required fields a pass over all of them, not one per new field', async () => { + const others = Array.from({ length: 100 }, (_, i) => ({ view: 'Input', name: `other${i}`, validate: 'required' })) + const meta = { view: 'Col', items: [{ view: 'Input', name: 'show', label: 'Show' }, ...others, { ...table, showIf: { name: 'show', equal: 'yes' } }] } + const values = { ...data(), ...Object.fromEntries(others.map(({ name }) => [name, 'x'])) } + render( {}} />) + const form = formHolding('show') + const shown = await costOf(() => form.change('show', 'yes')) + expect(form.getRegisteredFields()).toHaveLength(2 * ROWS + 102) + // One pass over the fields already there for the change, and one over them all when the bracket closes. + expect(shown).toEqual({ computed: 3230, validated: 301 }) + }) + + it('costs a popup that opens it beside 100 other required fields, and closes again, a pass over them each', async () => { + // Nothing mounts when a popup closes, whose node would bracket its fields going: the dialog brackets them. + const others = Array.from({ length: 100 }, (_, i) => ({ view: 'Input', name: `other${i}`, validate: 'required' })) + const meta = { + view: 'Col', + items: [ + { view: 'Button', children: 'Open details', onClick: { name: 'popupOpen', args: ['details'] } }, + ...others, + { view: 'Popup', id: 'details', title: 'Details', items: [table] }, + ], + } + const values = { ...data(), ...Object.fromEntries(others.map(({ name }) => [name, 'x'])) } + render( {}} />) + const form = formHolding('other0') + const opened = await costOf(() => fireEvent.click(screen.getByRole('button', { name: 'Open details' }))) + expect(form.getRegisteredFields()).toHaveLength(2 * ROWS + 101) + const closed = await costOf(() => fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' })) + expect(screen.queryByRole('dialog')).toBeNull() + expect(form.getRegisteredFields()).toHaveLength(100) + expect(opened).toEqual({ computed: 2212, validated: 201 }) + // Once its fields have gone, every field left is validated and told once. Before, each field that went + // validated and told every field left, 15150 validators in all, which React 16 still does. + expect(closed).toEqual(REMOVALS_BRACKETED ? { computed: 600, validated: 100 } : { computed: 60800, validated: 15150 }) + }) })