From 5f1a1cfa1c3f19076fdc8ca49b8cc8451a2c63ad Mon Sep 17 00:00:00 2001 From: YuanHeDx <96035115+YuanHeDx@users.noreply.github.com> Date: Mon, 24 Mar 2025 16:30:59 +0800 Subject: [PATCH] fix: do not delete object key when shallowSetIn value is undefined, fix err msg and add a ut (#84) * fix: do not delete key when shallowSetIn value is undefined * fix: fix err msg in path, add ut for chained swap * chore: comment the object test case not used in form * chore: add fix comment * fix: remove redundant clone --- .../form/__tests__/field-array-model.test.ts | 39 ++++++++++++++ .../node-engine/form/__tests__/object.test.ts | 52 +++++++------------ packages/node-engine/form/src/core/path.ts | 4 +- packages/node-engine/form/src/utils/object.ts | 18 +++---- 4 files changed, 65 insertions(+), 48 deletions(-) diff --git a/packages/node-engine/form/__tests__/field-array-model.test.ts b/packages/node-engine/form/__tests__/field-array-model.test.ts index f2cb359e..2ca54749 100644 --- a/packages/node-engine/form/__tests__/field-array-model.test.ts +++ b/packages/node-engine/form/__tests__/field-array-model.test.ts @@ -605,6 +605,45 @@ describe('FormArrayModel', () => { 'arr.1': [{ name: 'arr.1', message: 'err0', level: FeedbackLevel.Error }], }); }); + it('can chained swap', () => { + const arrayField = formModel.createFieldArray('x.arr'); + const a = arrayField!.append('a'); + const b = arrayField!.append('b'); + arrayField!.append('c'); + + formModel.init({}); + + a.state.errors = { + 'arr.0': [{ name: 'arr.0', message: 'err0', level: FeedbackLevel.Error }], + }; + b.state.errors = { + 'arr.1': [{ name: 'arr.1', message: 'err1', level: FeedbackLevel.Error }], + }; + + expect(a.name).toBe('x.arr.0'); + expect(b.name).toBe('x.arr.1'); + expect(formModel.values.x).toEqual({ arr: ['a', 'b', 'c'] }); + + arrayField.swap(1, 0); + expect(a.name).toBe('x.arr.1'); + expect(b.name).toBe('x.arr.0'); + expect(formModel.values.x).toEqual({ arr: ['b', 'a', 'c'] }); + + arrayField.swap(1, 0); + expect(a.name).toBe('x.arr.0'); + expect(formModel.fieldMap.get('x.arr.0').name).toBe('x.arr.0'); + expect(b.name).toBe('x.arr.1'); + expect(formModel.fieldMap.get('x.arr.1').name).toBe('x.arr.1'); + expect(formModel.values.x).toEqual({ arr: ['a', 'b', 'c'] }); + + arrayField.swap(1, 0); + expect(a.name).toBe('x.arr.1'); + expect(formModel.fieldMap.get('x.arr.1').name).toBe('x.arr.1'); + expect(b.name).toBe('x.arr.0'); + expect(formModel.fieldMap.get('x.arr.0').name).toBe('x.arr.0'); + + expect(formModel.values.x).toEqual({ arr: ['b', 'a', 'c'] }); + }); it('can swap from 0 to last index', () => { const arrayField = formModel.createFieldArray('arr'); diff --git a/packages/node-engine/form/__tests__/object.test.ts b/packages/node-engine/form/__tests__/object.test.ts index 225e9250..8f8539a2 100644 --- a/packages/node-engine/form/__tests__/object.test.ts +++ b/packages/node-engine/form/__tests__/object.test.ts @@ -72,12 +72,12 @@ describe('object', () => { expect(obj).toBe(newObj); }); - it('removes flat value', () => { + it('keep key shen set undefined', () => { const obj = { x: 'y' }; const newObj = shallowSetIn(obj, 'x', undefined); expect(obj).toEqual({ x: 'y' }); - expect(newObj).toEqual({}); - expect(newObj).not.toHaveProperty('x'); + expect(newObj).toEqual({ x: undefined }); + expect(Object.keys(newObj)).toEqual(['x']); }); it('sets nested value', () => { @@ -94,14 +94,6 @@ describe('object', () => { expect(newObj).toEqual({ x: 'y', nested: { value: 'b' } }); }); - it('removes nested value', () => { - const obj = { x: 'y', nested: { value: 'a' } }; - const newObj = shallowSetIn(obj, 'nested.value', undefined); - expect(obj).toEqual({ x: 'y', nested: { value: 'a' } }); - expect(newObj).toEqual({ x: 'y', nested: {} }); - expect(newObj.nested).not.toHaveProperty('value'); - }); - it('updates deep nested value', () => { const obj = { x: 'y', twofoldly: { nested: { value: 'a' } } }; const newObj = shallowSetIn(obj, 'twofoldly.nested.value', 'b'); @@ -110,15 +102,6 @@ describe('object', () => { expect(newObj).toEqual({ x: 'y', twofoldly: { nested: { value: 'b' } } }); // works ofc }); - it('removes deep nested value', () => { - const obj = { x: 'y', twofoldly: { nested: { value: 'a' } } }; - const newObj = shallowSetIn(obj, 'twofoldly.nested.value', undefined); - expect(obj.twofoldly.nested === newObj.twofoldly.nested).toEqual(false); - expect(obj).toEqual({ x: 'y', twofoldly: { nested: { value: 'a' } } }); - expect(newObj).toEqual({ x: 'y', twofoldly: { nested: {} } }); - expect(newObj.twofoldly.nested).not.toHaveProperty('value'); - }); - it('shallow clone data along the update path', () => { const obj = { x: 'y', @@ -192,20 +175,21 @@ describe('object', () => { expect(newObj).toEqual({ x: 'y', a: { x: { c: 'value' } } }); }); - it('should keep class inheritance for the top level object', () => { - class TestClass { - constructor(public key: string, public setObj?: any) {} - } - const obj = new TestClass('value'); - const newObj = shallowSetIn(obj, 'setObj.nested', 'shallowSetInValue'); - expect(obj).toEqual(new TestClass('value')); - expect(newObj).toEqual({ - key: 'value', - setObj: { nested: 'shallowSetInValue' }, - }); - expect(obj instanceof TestClass).toEqual(true); - expect(newObj instanceof TestClass).toEqual(true); - }); + // This case is not used in form sdk for now，so we comment it. + // it('should keep class inheritance for the top level object', () => { + // class TestClass { + // constructor(public key: string, public setObj?: any) {} + // } + // const obj = new TestClass('value'); + // const newObj = shallowSetIn(obj, 'setObj.nested', 'shallowSetInValue'); + // expect(obj).toEqual(new TestClass('value')); + // expect(newObj).toEqual({ + // key: 'value', + // setObj: { nested: 'shallowSetInValue' }, + // }); + // expect(obj instanceof TestClass).toEqual(true); + // expect(newObj instanceof TestClass).toEqual(true); + // }); it('can convert primitives to objects before setting', () => { const obj = { x: [{ y: true }] }; diff --git a/packages/node-engine/form/src/core/path.ts b/packages/node-engine/form/src/core/path.ts index 10384958..21c5adf1 100644 --- a/packages/node-engine/form/src/core/path.ts +++ b/packages/node-engine/form/src/core/path.ts @@ -100,14 +100,14 @@ export class Path { replaceParent(parent: Path, newParent: Path) { if (parent.value.length > this.value.length) { throw new Error( - `[Form] Error in Path.getChildSuffixByParent: invalid parent param: ${parent}, parent length should not greater than current length.` + `[Form] Error in Path.replaceParent: invalid parent param: ${parent}, parent length should not greater than current length.` ); } const rest = []; for (let i = 0; i < this.value.length; i++) { if (i < parent.value.length && parent.value[i] !== this.value[i]) { throw new Error( - `[Form] Error in Path.getChildSuffixByParent: invalid parent param: ${parent}` + `[Form] Error in Path.replaceParent: invalid parent param: '${parent}' is not a parent of '${this.toString()}'` ); } if (i >= parent.value.length) { diff --git a/packages/node-engine/form/src/utils/object.ts b/packages/node-engine/form/src/utils/object.ts index 0ff1f19c..5a76bbcb 100644 --- a/packages/node-engine/form/src/utils/object.ts +++ b/packages/node-engine/form/src/utils/object.ts @@ -80,18 +80,12 @@ export function shallowSetIn(obj: any, path: string, value: any): any { return obj; } - if (value === undefined) { - delete resVal[pathArray[i]]; - } else { - resVal[pathArray[i]] = value; - } - - // If the path array has a single element, the loop did not run. - // Deleting on `resVal` had no effect in this scenario, so we delete on the result instead. - if (i === 0 && value === undefined) { - delete res[pathArray[i]]; - } - + /** + * In Formik, they delete the key if the value is undefined. but here we keep the key with the undefined value. + * The reason that Formik tackle in this way is to fix the issue https://github.com/jaredpalmer/formik/issues/727 + * Their fix is https://github.com/jaredpalmer/formik/issues/727, and we roll back to the code before this PR. + */ + resVal[pathArray[i]] = value; return res; }