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

[oluwatimio]

Description

Fixes #1830 and part of https://github.com/Shopify/core-issues/issues/23975

There is no reset ability for dynamic list or flag to know if the dynamic list is dirty. This means if for example we are using a context bar and click discard, the dynamic list new field will still persist.

This PR adds the ability to reset to the initial list state and to know if the dynamic list is dirty.

It also adds the ability to add multiple dynamic lists

co-authored with @sylvhama

Tophat

Code sandbox here .

Type of change

• react-form/dynamic-list Minor: New feature (non-breaking change which adds functionality)

Checklist

• I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

[kaelig]

👋 Thank you for the PR!

Please run:

git pull origin main --rebase && git push origin dynamic-lists/add-reset-and-dirty-state --force

to get the latest changes and fix the failing test, apologies for the inconvenience.

[sylvhama]

I have tested locally via yalc and it does not fix #1830
It seems useForm is not aware that the dynamic list is dirty. If I add a new item the form remains not dirty. Plus when I press reset I would expect that the list becomes empty again but it does not.

[oluwatimio]

With this commit 8a4cfa7 we can now reset and get the dirty state of the dynamic list from the forms reset and dirty state

[oluwatimio]

PR now ready for review. Resetting lists in a listbag with the form's reset function should now be possible with dynamic list or any type of list created in the future. Same case for dirty.

[sylvhama]

My biggest concern is the comment about export interface ListBag.
Otherwise it looks good, I have not checked all the tests since we might update several stuff first.

[marutypes]

One thing I noticed that was already there, an a comment about some readability concerns, but otherwise this is looking pretty nice to me! I didn't tophat, but I'm happy to have this API in the library.

[marutypes]

I know this was already here, but a note on the pattern:

This technically works but may stop working in future react updates. We recently removed similar patterns in other hooks in quilt and are considering adding a lint rule. It might be best to find an alternative way of doing this as a result.

Actually, looking at it closer, since we update it literally every render, why can't we just use the value directly?

[oluwatimio]

Ahh yes I think we can get rid of the ref since we update it, Thanks! Will try that

[oluwatimio]

Noticing removing the ref causes the makeClean function to be re-created on every render so I switched to useLazyRef to avoid mutating refs directly during the render phase

[attila-berki]

LGTM, nice improvement of the form and the dynamic list 💯

[sylvhama]

I have tested it in web and it works! However I have noticed a typing issue that should be addressed before merging I think:

In your codesandbox you use ! but I think consumers should not have to do this. We need to explore if we can tell TS to detect we have passed dynamic list(s) 🤔

[oluwatimio]

I have tested it in web and it works! However I have noticed a typing issue that should be addressed before merging I think:

In your codesandbox you use ! but I think consumers should not have to do this. We need to explore if we can tell TS to detect we have passed dynamic list(s) 🤔

Hmm, I see 🤔 . It is because the parameter is optional that dynamiclist can be undefined in the forms return. We will always need to do this / check first.

It is like having a prop that can possibly be undefined, we will need to check if it is undefined before actually accessing this property. I am not sure I can see a way around this except retuning an empty dynamic list if dynamiclist param is undefined. I personally do not recommend masking the fact that it can be undefined just to avoid having to check.

[sylvhama]

I think there must be a way e.g. https://www.typescriptlang.org/docs/handbook/2/conditional-types.html (I am not TS expert, this might not be the right thing to look at).

Also another problem comes to my mind, what if I only one to manipulate dynamic lists? then fields should become optional 😬

[oluwatimio]

I have tested it in web and it works! However I have noticed a typing issue that should be addressed before merging I think:

In your codesandbox you use ! but I think consumers should not have to do this. We need to explore if we can tell TS to detect we have passed dynamic list(s) 🤔

In my opinion, the point of the non-null assertion operator is to tell the TypeScript compiler that you already know the variable is not null. I dont see any disadvantages to using it if you know that the dynamic list was passed in

[oluwatimio]

I think there must be a way e.g. https://www.typescriptlang.org/docs/handbook/2/conditional-types.html (I am not TS expert, this might not be the right thing to look at).

Also another problem comes to my mind, what if I only one to manipulate dynamic lists? then fields should become optional 😬

I am not sure but I think if we are to use a conditional type, wouldn't we still have to check what type it is before using the dynamic list? I personally don't see this being different to a prop being optional in react and having to check its defined before using it. Same thing for a gql query retuning an optional value and having to check. I am not sure where the problem lies because the user is in control of passing in the dynamic list so if they know it is defined, they can use the non null assertation which is what it was created for. The typescript docs says that here

[oluwatimio]

Another thing I noticed is, if we are to change it from non-optional to required in the Form's interface, we will have a syntax error

In this case, we may have to modify the parameter as well to be able to assign the dynamicList in the return. This means that we either make the parameter required (which will break everything in web) or we can assign a default value. Also, as for the field parameter, it is implied that we cannot use useForm without a field because the field parameter is required. I would probably suggest that if someone wants to use useForm and dynamic list without a field, they can use the optional way of initializing a dynamic list which is to pass the FieldDictionary as a field into the form (the way its currently being done). Then use the reset and dirty states of the dynamic list.

[oluwatimio]

We discussed this and we will be returning an empty DynamicList bag when dynamicLists param is undefined. This will have no effect on computation in the form and will remove having to use the assertion operator or checking if dynamic list is defined.

[sylvhama]

Proposition to avoid default value for dynamicLists: #1848