From 0b8a3561c97224de9cd57828bead7b5426ec4af1 Mon Sep 17 00:00:00 2001 From: Pavel Denisjuk Date: Tue, 14 May 2024 11:59:25 +0200 Subject: [PATCH] fix(form): initialize form on first render [skip ci] --- packages/form/__tests__/form.test.tsx | 59 ++++++++++++++++++++++++--- packages/form/src/Form.tsx | 42 ++++++++++++------- packages/form/src/FormPresenter.ts | 25 ++++-------- packages/form/src/useBind.ts | 5 --- 4 files changed, 91 insertions(+), 40 deletions(-) diff --git a/packages/form/__tests__/form.test.tsx b/packages/form/__tests__/form.test.tsx index fb57ace5b2c..962d04f0d16 100644 --- a/packages/form/__tests__/form.test.tsx +++ b/packages/form/__tests__/form.test.tsx @@ -1,10 +1,17 @@ /** * @jest-environment jsdom */ -import React, { useState } from "react"; +import React from "react"; import { render, screen, waitFor, fireEvent, act } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { Form, useBind, FormProps as BaseFormProps, BindComponentProps, FormAPI } from "~/index"; +import { + Form, + useBind, + FormProps as BaseFormProps, + BindComponentProps, + FormAPI, + Bind +} from "~/index"; import { validation } from "@webiny/validation"; type FormProps = Omit & { @@ -13,8 +20,28 @@ type FormProps = Omit & { children?: React.ReactNode; }; +const EmptyForm = ({ children, data, onSubmit, imperativeHandle, ...props }: FormProps) => { + const formData = data; + + return ( +
onSubmit && onSubmit(data)} + {...props} + > + {({ form }) => ( +
+ {children} + +
+ )} +
+ ); +}; + const FormViewWithBind = ({ children, data, onSubmit, imperativeHandle, ...props }: FormProps) => { - const [formData] = useState(data || { name: "empty name" }); + const formData = data || { name: "empty name" }; return (
{ const user = userEvent.setup(); const onSubmit = jest.fn(); - const { rerender } = render(); + const { rerender } = render(); const submitBtn = screen.getByRole("button", { name: /submit/i }); await user.click(submitBtn); expect(onSubmit).toHaveBeenLastCalledWith({}); - rerender(); + rerender(); await user.click(submitBtn); await waitFor(() => onSubmit.mock.calls.length > 0); expect(onSubmit).toHaveBeenLastCalledWith({ email: "test@example.com" }); }); + test("should set default field value on first render cycle", async () => { + const ref = React.createRef(); + const onSubmit = jest.fn(); + + render( + + + {({ value }) =>
{value.id}
} + + + ); + + // Assert + await act(() => ref.current?.submit()); + await waitFor(() => onSubmit.mock.calls.length > 0); + expect(onSubmit).toHaveBeenLastCalledWith({ folder: { id: "root" } }); + + const folderDiv = screen.getByTestId("folderId"); + expect(folderDiv).toBeTruthy(); + expect(folderDiv.innerHTML).toEqual("root"); + }); + test("should submit the form using imperative handle", async () => { const user = userEvent.setup(); const ref = React.createRef(); diff --git a/packages/form/src/Form.tsx b/packages/form/src/Form.tsx index 4d1420c7345..6d8573b9d87 100644 --- a/packages/form/src/Form.tsx +++ b/packages/form/src/Form.tsx @@ -1,6 +1,8 @@ -import React, { useEffect, useImperativeHandle, useMemo } from "react"; +import React, { useEffect, useImperativeHandle, useMemo, useRef } from "react"; import { observer } from "mobx-react-lite"; import lodashNoop from "lodash/noop"; +import isEqual from "lodash/isEqual"; + import { Bind } from "./Bind"; import { FormProps, GenericFormData } from "~/types"; import { FormContext } from "./FormContext"; @@ -11,7 +13,22 @@ function FormInner( props: FormProps, ref: React.ForwardedRef ) { - const presenter = useMemo(() => new FormPresenter(), []); + const dataRef = useRef(props.data); + + const presenter = useMemo(() => { + const presenter = new FormPresenter(); + presenter.init({ + data: (props.data || {}) as T, + onChange: data => { + if (typeof props.onChange === "function") { + props.onChange(data, formApi); + } + }, + onInvalid: props.onInvalid + }); + return presenter; + }, []); + const formApi = useMemo(() => { return new FormAPI(presenter, { onSubmit: props.onSubmit ?? lodashNoop, @@ -28,24 +45,21 @@ function FormInner( }); }, [props.onSubmit, props.disabled, props.validateOnFirstSubmit]); - useEffect(() => { - presenter.init({ - data: (props.data || {}) as T, - onChange: data => { - if (typeof props.onChange === "function") { - props.onChange(data, formApi); - } - }, - onInvalid: props.onInvalid - }); - }, []); - useEffect(() => { presenter.setInvalidFields(props.invalidFields || {}); }, [props.invalidFields]); useEffect(() => { + // We only set form's data if props.data has changed. + if (isEqual(dataRef.current, props.data)) { + return; + } + + // Set the new form data. presenter.setData(props.data as T); + + // Keep the new props.data for future comparison. + dataRef.current = props.data; }, [props.data]); useImperativeHandle(ref, () => ({ diff --git a/packages/form/src/FormPresenter.ts b/packages/form/src/FormPresenter.ts index ec5aaabe830..7007750271e 100644 --- a/packages/form/src/FormPresenter.ts +++ b/packages/form/src/FormPresenter.ts @@ -143,31 +143,24 @@ export class FormPresenter { registerField(props: BindComponentProps) { const existingField = this.formFields.get(props.name); - let field; + let field: FormField; if (existingField) { field = FormField.createFrom(existingField, props); } else { field = FormField.create(props); } - this.formFields.set(props.name, field); - // We only want to handle default field value for new fields. - if (existingField) { - return; + if (!existingField) { + const fieldName = field.getName(); + const currentFieldValue = lodashGet(this.data, fieldName); + const defaultValue = field.getDefaultValue(); + if (currentFieldValue === undefined && defaultValue !== undefined) { + lodashSet(this.data, fieldName, defaultValue); + } } - // Set field's default value. - const fieldName = field.getName(); - const currentFieldValue = lodashGet(this.data, fieldName); - const defaultValue = field.getDefaultValue(); - if (currentFieldValue === undefined && defaultValue !== undefined) { - // We need to postpone the state update, because `registerField` is called within a render cycle. - // You can't set a new state, while the previous state is being rendered. - requestAnimationFrame(() => { - this.setFieldValue(fieldName, defaultValue); - }); - } + this.formFields.set(props.name, field); } unregisterField(name: string) { diff --git a/packages/form/src/useBind.ts b/packages/form/src/useBind.ts index e480d332a98..a5cb9ae0f11 100644 --- a/packages/form/src/useBind.ts +++ b/packages/form/src/useBind.ts @@ -1,6 +1,5 @@ import { useEffect } from "react"; import { makeDecoratable } from "@webiny/react-composition"; -import lodashGet from "lodash/get"; import { BindComponentProps, UseBindHook } from "~/types"; import { useBindPrefix } from "~/BindPrefix"; import { useForm } from "./FormContext"; @@ -14,10 +13,6 @@ export const useBind = makeDecoratable((props: BindComponentProps): UseBindHook const bindName = [bindPrefix, props.name].filter(Boolean).join("."); useEffect(() => { - if (props.defaultValue !== undefined && lodashGet(form.data, bindName) === undefined) { - form.setValue(bindName, props.defaultValue); - } - return () => { form.unregisterField(props.name); };