improve dynamicLists typing #1848

sylvhama · 2021-04-20T18:03:46Z

You can test with this condesandbox.
I am using function overloading to define two scenarios:

we have a classic useForm (like all existing ones) that does not use dynamicLists;
we can a useForm relying dynamicLists.

Result is:

No more default value in our hook;
In App.tsx: No need to use ! or to check if dynamicLists is defined.

In the future we could do the same to also turn fields optional 🤔

oluwatimio · 2021-04-20T18:31:54Z

packages/react-form/src/hooks/form.ts

+  makeCleanAfterSubmit,
+}: FormWithDynamicListsInput<T, D>): FormWithDynamicLists<T, D>;
+
+export function useForm<T extends FieldBag, D extends DynamicListBag>({


I am wondering, would you have to pass in a type for the second part of the generic for every useForm call that already passes in a generic type for the form?

nope, as you can see in the codesandbox (App.tsx), I made two form and I don't need to pass the generic types.

oluwatimio

I think this is good! Nice 💯 . Thanks for digging into it 🙂 . I am a bit concerned about modifying the useForm this way but it's looking good in the sandbox!

I also noticed all the tests in form.test.tsx are breaking. The full CI does not seem to be running in this PR for some reason but you can try it locally. I can fix the tests in my PR so dont worry about it.

sylvhama · 2021-04-21T14:00:26Z

Aaaaah I thought it was all green LOL nice catch! I will run tests locally and see what's going on.

oluwatimio · 2021-04-21T14:02:23Z

LOL nice catch! I will run tests locally a

Its all good! We just have to change the useForm call to something like this

    const {
      submit,
      submitting,
      dirty,
      reset,
      makeClean,
      submitErrors,
      dynamicLists,
    } = useForm({
      fields: {title, description, defaultVariant, variants},
      dynamicLists: {
        dynamicVariants: useDynamicList(data.variants, variantsEmptyFactory),
      },
      onSubmit: onSubmit as any,
      makeCleanAfterSubmit,
    });

    const {
      dynamicVariants: {addItem, fields: dynamicListFields},
    } = dynamicLists;

and maybe remove the enable dynamic list prop in productForm and then remove the default test

attila-berki

🤯🤯🤯

sylvhama · 2021-04-21T18:44:41Z

packages/react-form/src/hooks/list/test/dynamiclist.test.tsx

@@ -73,6 +79,9 @@ describe('useDynamicList', () => {
    });

    it('will throw an error if new position is out of index range', () => {
+      const consoleErrorMock = jest.spyOn(console, 'error');
+      consoleErrorMock.mockImplementation();


sylvhama · 2021-04-21T18:45:12Z

packages/react-form/src/hooks/list/test/dynamiclist.test.tsx

@@ -203,6 +212,66 @@ describe('useDynamicList', () => {
        expect(wrapper).toContainReactComponent(TextField);
      });
    });
+
+    describe('dirty dynamic list', () => {


So far there was no dirty related test

hmm 🤔 @sylvhama . There are dirty tests in the baselist since that is where it is originally implemented

So I dont think we will need to have it in the dynamiclist test since dynamic list just returns what the baselist is returning

same case for reset

But useList test don't cover cases where we add or remove an item. Plus it does not hurt to make sure nothing break the default behavior.

Oh I dont meanuseList, I meant useBaseList. Because I noticed in this PR some tests like value changes are being re-created. In that case, could we just write the tests that cover the adding and removing?

but addItem, moveItem, removeItem can only be tested here since they are added by dynamiclist.ts. Also we want to make sure there is no local logic within dynamiclist.ts that could break the dirty logic 🤔

sylvhama · 2021-04-21T18:46:23Z

packages/react-form/src/hooks/test/form-with-dynamic-list.test.tsx

+
+import {submitSuccess, submitFail} from '..';
+
+describe('useForm with dynamic list', () => {


those tests are inspired by origin useForm test.

makeClean and makeCleanAfterSubmit seem broken with dynamic lists 🤔

sylvhama · 2021-04-21T18:46:58Z

packages/react-form/src/hooks/test/form.test.tsx

+  waitForSubmit,
+} from './utilities';
+
+import {submitSuccess, submitFail} from '..';

 describe('useForm', () => {


All the code removed here has been moved to utilities.

sylvhama · 2021-04-21T18:47:14Z

packages/react-form/src/hooks/test/utilities/components/FormWithDynamicVariantList.tsx

+import {SimpleProduct, Variant} from './types';
+import {TextField} from './TextField';
+
+export function FormWithDynamicVariantList({


This is a new component to test dynamic lists.

sylvhama · 2021-04-21T18:47:27Z

packages/react-form/src/hooks/test/utilities/components/ProductForm.tsx

+import {SimpleProduct} from './types';
+import {TextField} from './TextField';
+
+export function ProductForm({


Nothing changed here, I just moved the code.

* improve dynamicLists typing * revert to initial tests * mock console.error to clean logs * add dirty tests for dynamic lists * update form tests * remove useless file

test baselist Update baselist.test.tsx lint fix and additional tests update tests Update CHANGELOG.md Update README.md Update CHANGELOG.md add dynamic list to the form update comments Update form.ts add check for input since theres a possibility of adding undefined input dynamicListFields Update baselist.ts Update baselist.ts added tests to form Update form.test.tsx fixed reset recreating on each render Update listdirty.ts Update README.md Update README.md Update README.md added DynamicListBag Update packages/react-form/README.md Co-authored-by: Sylvain Hamann <[email protected]> Update packages/react-form/README.md Co-authored-by: Sylvain Hamann <[email protected]> Update README.md Update baselist.ts docs used lazy ref fix docs more improvement to docs return default bag on undefined dynamic list Update form.test.tsx rebase gone bad fix improve dynamicLists typing (#1848) * improve dynamicLists typing * revert to initial tests * mock console.error to clean logs * add dirty tests for dynamic lists * update form tests * remove useless file Update dynamiclist.test.tsx

improve dynamicLists typing

45aa9ac

sylvhama requested review from a team, attila-berki and oluwatimio and removed request for a team April 20, 2021 18:03

sylvhama mentioned this pull request Apr 20, 2021

[react-form | DynamicList] add reset function and dirty state #1828

Merged

2 tasks

oluwatimio reviewed Apr 20, 2021

View reviewed changes

oluwatimio approved these changes Apr 21, 2021

View reviewed changes

attila-berki approved these changes Apr 21, 2021

View reviewed changes

sylvhama added 5 commits April 21, 2021 10:38

revert to initial tests

34610be

mock console.error to clean logs

e450856

add dirty tests for dynamic lists

aa57d27

update form tests

715607c

remove useless file

ee11f9b

sylvhama commented Apr 21, 2021

View reviewed changes

sylvhama merged commit 6f19a13 into dynamic-lists/add-reset-and-dirty-state Apr 21, 2021

sylvhama deleted the react-form-dynamic-lists-typing branch April 21, 2021 19:33

oluwatimio pushed a commit that referenced this pull request Apr 21, 2021

improve dynamicLists typing (#1848)

0345f6e

* improve dynamicLists typing * revert to initial tests * mock console.error to clean logs * add dirty tests for dynamic lists * update form tests * remove useless file

sylvhama added the Package: react-form label Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve dynamicLists typing #1848

improve dynamicLists typing #1848

sylvhama commented Apr 20, 2021

oluwatimio Apr 20, 2021 •

edited

Loading

sylvhama Apr 20, 2021

oluwatimio left a comment

sylvhama commented Apr 21, 2021

oluwatimio commented Apr 21, 2021 •

edited

Loading

attila-berki left a comment

sylvhama Apr 21, 2021

sylvhama Apr 21, 2021

oluwatimio Apr 21, 2021

oluwatimio Apr 21, 2021 •

edited

Loading

oluwatimio Apr 21, 2021

sylvhama Apr 21, 2021

oluwatimio Apr 21, 2021

sylvhama Apr 21, 2021

sylvhama Apr 21, 2021

sylvhama Apr 21, 2021

sylvhama Apr 21, 2021

sylvhama Apr 21, 2021


		import {submitSuccess, submitFail} from '..';

		describe('useForm with dynamic list', () => {

improve dynamicLists typing #1848

improve dynamicLists typing #1848

Conversation

sylvhama commented Apr 20, 2021

oluwatimio Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oluwatimio left a comment

Choose a reason for hiding this comment

sylvhama commented Apr 21, 2021

oluwatimio commented Apr 21, 2021 • edited Loading

attila-berki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oluwatimio Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oluwatimio Apr 20, 2021 •

edited

Loading

oluwatimio commented Apr 21, 2021 •

edited

Loading

oluwatimio Apr 21, 2021 •

edited

Loading