Skip to content

Commit c52d897

Browse files
author
erikras-dinesh-agent
committed
Merge main into fix/issue-1050-usefield-undefined-initial
Resolved merge conflicts: - src/useField.ts: Kept useSyncExternalStore-based implementation - src/useField.test.js: Merged additional test cases from main - src/Field.test.js: Merged comment improvements from main - yarn.lock: Regenerated after merge - Added type declaration for use-sync-external-store/shim - Fixed TypeScript errors for component type and getIn parameters
2 parents a708212 + 19cbe2c commit c52d897

13 files changed

Lines changed: 1899 additions & 2044 deletions

eslint.config.js renamed to eslint.config.mjs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,21 @@ export default [
161161
},
162162
},
163163

164+
// Configuration for .mjs files (ES modules in Node.js environment)
165+
{
166+
files: ["**/*.mjs"],
167+
languageOptions: {
168+
ecmaVersion: "latest",
169+
sourceType: "module",
170+
globals: {
171+
...nodeGlobals,
172+
},
173+
},
174+
rules: {
175+
"no-undef": "error",
176+
},
177+
},
178+
164179
// Specific overrides for ALL test files (JS and TS)
165180
{
166181
files: ["**/*.test.js", "**/*.test.jsx", "**/*.test.ts", "**/*.test.tsx"],

package-scripts.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ module.exports = {
1616
),
1717
size: {
1818
description: "check the size of the bundle",
19-
script: "bundlesize",
19+
script: "size-limit",
2020
},
2121
},
2222
build: {

package.json

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,18 +42,19 @@
4242
"@rollup/plugin-node-resolve": "^15.2.4",
4343
"@rollup/plugin-replace": "^5.0.7",
4444
"@rollup/plugin-terser": "^0.4.4",
45+
"@size-limit/preset-small-lib": "^11.1.6",
4546
"@testing-library/dom": "^10.4.0",
4647
"@testing-library/jest-dom": "^6.6.3",
4748
"@testing-library/react": "^16.3.0",
4849
"@types/node": "^20.17.50",
4950
"@types/react": "^19.1.5",
5051
"@types/react-dom": "^19.1.5",
52+
"@types/use-sync-external-store": "^1.5.0",
5153
"@typescript-eslint/eslint-plugin": "^8.32.1",
5254
"@typescript-eslint/parser": "^8.32.1",
5355
"babel-core": "^7.0.0-bridge.0",
5456
"babel-eslint": "^10.1.0",
5557
"babel-jest": "^29.7.0",
56-
"bundlesize": "^0.18.2",
5758
"doctoc": "^2.2.1",
5859
"dtslint": "^4.2.1",
5960
"eslint": "^9.27.0",
@@ -78,6 +79,7 @@
7879
"react": "^19.1.0",
7980
"react-dom": "^19.1.0",
8081
"rollup": "^3.29.5",
82+
"size-limit": "^11.1.6",
8183
"tar": "^7.4.3",
8284
"ts-essentials": "^10.0.4",
8385
"tslint": "^6.1.3",
@@ -100,18 +102,18 @@
100102
"/typescript/"
101103
]
102104
},
103-
"bundlesize": [
105+
"size-limit": [
104106
{
105107
"path": "dist/react-final-form.umd.min.js",
106-
"threshold": "2kB"
108+
"limit": "4kB"
107109
},
108110
{
109111
"path": "dist/react-final-form.es.js",
110-
"threshold": "3kB"
112+
"limit": "4kB"
111113
},
112114
{
113115
"path": "dist/react-final-form.cjs.js",
114-
"threshold": "3kB"
116+
"limit": "4kB"
115117
}
116118
],
117119
"collective": {
File renamed without changes.

src/Field.test.js

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -801,16 +801,18 @@ describe("Field", () => {
801801
);
802802
expect(red).toHaveBeenCalled();
803803
expect(red).toHaveBeenCalledTimes(2);
804-
expect(red.mock.calls[0][0].input.checked).toBe(true); // Now correctly true on first render!
805-
expect(red.mock.calls[1][0].input.checked).toBe(true); // Still true for "red" checkbox
804+
// After fix #1050, initialValues work on first render
805+
expect(red.mock.calls[0][0].input.checked).toBe(true); // Correctly true for "red" from initialValues
806+
expect(red.mock.calls[1][0].input.checked).toBe(true);
806807
expect(green).toHaveBeenCalled();
807808
expect(green).toHaveBeenCalledTimes(2);
808-
expect(green.mock.calls[0][0].input.checked).toBe(false); // Correctly false on first render
809-
expect(green.mock.calls[1][0].input.checked).toBe(false); // Still false for "green" checkbox
809+
expect(green.mock.calls[0][0].input.checked).toBe(false);
810+
expect(green.mock.calls[1][0].input.checked).toBe(false); // Correctly false for "green"
810811
expect(blue).toHaveBeenCalled();
811812
expect(blue).toHaveBeenCalledTimes(2);
812-
expect(blue.mock.calls[0][0].input.checked).toBe(true); // Now correctly true on first render!
813-
expect(blue.mock.calls[1][0].input.checked).toBe(true); // Still true for "blue" checkbox
813+
// After fix #1050, initialValues work on first render
814+
expect(blue.mock.calls[0][0].input.checked).toBe(true); // Correctly true for "blue" from initialValues
815+
expect(blue.mock.calls[1][0].input.checked).toBe(true);
814816
});
815817

816818
it("should render radio buttons with checked prop", () => {
@@ -880,16 +882,17 @@ describe("Field", () => {
880882
);
881883
expect(red).toHaveBeenCalled();
882884
expect(red).toHaveBeenCalledTimes(2);
883-
expect(red.mock.calls[0][0].input.checked).toBe(false); // Correctly false on first render
884-
expect(red.mock.calls[1][0].input.checked).toBe(false); // Still false for "red" radio
885+
expect(red.mock.calls[0][0].input.checked).toBe(false);
886+
expect(red.mock.calls[1][0].input.checked).toBe(false); // Correctly false for "red" radio
885887
expect(green).toHaveBeenCalled();
886888
expect(green).toHaveBeenCalledTimes(2);
887-
expect(green.mock.calls[0][0].input.checked).toBe(true); // Now correctly true on first render!
888-
expect(green.mock.calls[1][0].input.checked).toBe(true); // Still true for "green" radio
889+
// After fix #1050, initialValues work on first render
890+
expect(green.mock.calls[0][0].input.checked).toBe(true); // Correctly true for "green" from initialValues
891+
expect(green.mock.calls[1][0].input.checked).toBe(true);
889892
expect(blue).toHaveBeenCalled();
890893
expect(blue).toHaveBeenCalledTimes(2);
891-
expect(blue.mock.calls[0][0].input.checked).toBe(false); // Correctly false on first render
892-
expect(blue.mock.calls[1][0].input.checked).toBe(false); // Still false for "blue" radio
894+
expect(blue.mock.calls[0][0].input.checked).toBe(false);
895+
expect(blue.mock.calls[1][0].input.checked).toBe(false); // Correctly false for "blue" radio
893896
});
894897

895898
it("should use isEqual to calculate dirty/pristine", () => {
@@ -967,8 +970,6 @@ describe("Field", () => {
967970
)}
968971
</Form>,
969972
);
970-
// With the useSyncExternalStore fix for #1050, validation runs once
971-
// during the initial field registration
972973
expect(fooValidate).toHaveBeenCalledTimes(1);
973974
expect(barValidate).toHaveBeenCalledTimes(1);
974975
expect(bazValidate).toHaveBeenCalledTimes(1);
@@ -1010,8 +1011,9 @@ describe("Field", () => {
10101011
</Form>,
10111012
);
10121013

1013-
// With the fix for #1050, initialValues are now correctly available on first render,
1014-
// so the select multiple value IS an array from the start - no warning needed!
1014+
// After fix #1050, initialValues work on first render, so select multiple
1015+
// correctly gets the array value from initialValues and no longer triggers
1016+
// React's "must be an array" warning
10151017
expect(errorSpy).toHaveBeenCalledTimes(0);
10161018

10171019
// Reset the spy to test the actual Field warnings

src/FormSpy.test.js

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,4 +416,47 @@ describe("FormSpy", () => {
416416
// Implementation of this test case is not provided in the original file or the code block
417417
// This test case is assumed to exist based on the code block
418418
});
419+
420+
it("should not throw error when subscribing to active (fix #1055)", () => {
421+
// Issue #1055: Cannot set property active of #<Object> which has only a getter
422+
// This was caused by spreading lazy state with getter-only properties into renderProps
423+
const spy = jest.fn();
424+
const { container, getByTestId } = render(
425+
<Form onSubmit={onSubmitMock}>
426+
{() => (
427+
<form>
428+
<Field name="name" component="input" data-testid="name" />
429+
<FormSpy
430+
subscription={{ active: true, values: true }}
431+
render={wrapWith(spy, (props) => {
432+
// Verify that we can access active and values without error
433+
const { active, values } = props;
434+
return <div data-testid="spy">active={String(active)} values={JSON.stringify(values)}</div>;
435+
})}
436+
/>
437+
</form>
438+
)}
439+
</Form>,
440+
);
441+
442+
// Should render without throwing "Cannot set property active" error
443+
expect(container).toBeTruthy();
444+
expect(spy).toHaveBeenCalled();
445+
446+
// Verify that active and values properties exist and can be accessed
447+
const props = spy.mock.calls[spy.mock.calls.length - 1][0];
448+
expect("active" in props).toBe(true);
449+
expect("values" in props).toBe(true);
450+
451+
// Active should be undefined initially (no field focused)
452+
expect(props.active).toBeUndefined();
453+
454+
// Values should be defined (empty object initially)
455+
expect(props.values).toBeDefined();
456+
expect(props.values).toEqual({});
457+
458+
// When we focus a field, active should be the field name
459+
fireEvent.focus(getByTestId("name"));
460+
expect(spy.mock.calls[spy.mock.calls.length - 1][0].active).toBe("name");
461+
});
419462
});

src/FormSpy.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ function FormSpy<FormValues = Record<string, any>>({
1616
}
1717

1818
const renderProps: FormSpyRenderProps<FormValues> = {
19-
...state,
2019
form: {
2120
...reactFinalForm,
2221
reset: (eventOrValues?: any) => {

src/renderComponent.test.js

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,4 +69,76 @@ describe("renderComponent", () => {
6969
expect(getA).not.toHaveBeenCalled();
7070
expect(getB).not.toHaveBeenCalled();
7171
});
72+
73+
it("should not overwrite getter-only properties when using component prop", () => {
74+
// This test reproduces issue #1055
75+
// When lazyProps has getter-only properties (like 'active'), and props contains
76+
// a property with the same name, it should not attempt to overwrite the getter
77+
const Component = () => null;
78+
const props = {
79+
component: Component,
80+
active: "value-from-props", // This would cause "Cannot set property active" error
81+
customProp: "custom",
82+
};
83+
const lazyProps = {};
84+
Object.defineProperty(lazyProps, "active", {
85+
get: () => "value-from-getter",
86+
enumerable: true,
87+
// Note: no setter - this is getter-only
88+
});
89+
const name = "TestComponent";
90+
91+
// Should not throw "Cannot set property active"
92+
let result;
93+
expect(() => {
94+
result = renderComponent(props, lazyProps, name);
95+
}).not.toThrow();
96+
97+
// Check the React element was created with correct props
98+
expect(result.type).toBe(Component);
99+
100+
// The getter-only property should remain and use the getter value
101+
expect(result.props.active).toBe("value-from-getter");
102+
103+
// Custom props should still be passed through
104+
expect(result.props.customProp).toBe("custom");
105+
});
106+
107+
it("should handle getter-only properties in all render paths", () => {
108+
const lazyProps = {};
109+
Object.defineProperty(lazyProps, "active", {
110+
get: () => "getter-value",
111+
enumerable: true,
112+
});
113+
114+
// Test with render prop
115+
const render = jest.fn();
116+
renderComponent(
117+
{ render, active: "prop-value" },
118+
lazyProps,
119+
"TestComponent",
120+
);
121+
expect(render).toHaveBeenCalled();
122+
expect(render.mock.calls[0][0].active).toBe("getter-value");
123+
124+
// Test with children function
125+
const children = jest.fn();
126+
renderComponent(
127+
{ children, active: "prop-value" },
128+
lazyProps,
129+
"TestComponent",
130+
);
131+
expect(children).toHaveBeenCalled();
132+
expect(children.mock.calls[0][0].active).toBe("getter-value");
133+
134+
// Test with component prop
135+
const Component = () => null;
136+
const result = renderComponent(
137+
{ component: Component, active: "prop-value" },
138+
lazyProps,
139+
"TestComponent",
140+
);
141+
expect(result.type).toBe(Component);
142+
expect(result.props.active).toBe("getter-value");
143+
});
72144
});

src/renderComponent.ts

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,20 @@ export default function renderComponent<T>(
1010
): React.ReactNode {
1111
const { render, children, component, ...rest } = props;
1212
if (component) {
13-
return React.createElement(
14-
component,
15-
Object.assign(lazyProps, rest, {
16-
children,
17-
render,
18-
}),
19-
);
13+
// FIX: Don't use Object.assign which tries to overwrite getters
14+
// Instead, create a new object with lazyProps descriptors first,
15+
// then add non-conflicting properties from rest
16+
const result = {} as any;
17+
Object.defineProperties(result, Object.getOwnPropertyDescriptors(lazyProps));
18+
const restDescriptors = Object.getOwnPropertyDescriptors(rest);
19+
for (const key in restDescriptors) {
20+
if (!(key in result)) {
21+
Object.defineProperty(result, key, restDescriptors[key]);
22+
}
23+
}
24+
result.children = children;
25+
result.render = render;
26+
return React.createElement(component, result);
2027
}
2128
if (render) {
2229
const result = {} as T;
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
declare module 'use-sync-external-store/shim' {
2+
export function useSyncExternalStore<Snapshot>(
3+
subscribe: (onStoreChange: () => void) => () => void,
4+
getSnapshot: () => Snapshot,
5+
getServerSnapshot?: () => Snapshot,
6+
): Snapshot;
7+
}

0 commit comments

Comments
 (0)