Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes for svelte's useForm helper #1610

Merged
merged 4 commits into from Aug 9, 2023

Conversation

edgars-vasiljevs
Copy link
Contributor

@edgars-vasiljevs edgars-vasiljevs commented Jul 13, 2023

This fixes two issues in useForm helper that I found.

  • Form fields with objects as values (arrays/objects) has broken reactivity. This is because input data is shallow copied to defaults. Updating nested objects in form updates defaults object as well. Since the current form data is compared to defaults, no changes are detected.
  • Broken setStore. Svelte's stores replaces store value with anything it receives from update's callback function. In this case setStore creates completely new object instance every time it's called by returning Object.assign({}, store, ...). Later when form submit fails, this.clearErrors().setError(errors) is called on object instance that request was made from which is NOT the object currently stored in form store hence nothing is cleared.

This should fix form error issue in #1521 #1276

Edit:

Added two more fixes:

  • reset function also shallow copies but this time defaults to data. This again breaks nested array/object reactivity.
    - updating defaults to current data after successful request. It's debatable but IMO form defaults should be updated because previous form state is irrelevant after successful submit.

I see that Vue adapters work very similarly to the changes I made.

@taylorotwell
Copy link

Hey there - this is on our radar. Hoping to get it merged next week. 👍

@jessarcher jessarcher self-requested a review August 9, 2023 01:46
Copy link
Member

@jessarcher jessarcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this locally with and without this branch and can confirm the issue and the fix.

Thank you!

@jessarcher jessarcher merged commit ea6a634 into inertiajs:master Aug 9, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants