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

Update useForm to correctly update/read values in Svelte store #1574

Closed
wants to merge 1 commit into from
Closed

Update useForm to correctly update/read values in Svelte store #1574

wants to merge 1 commit into from

Conversation

daleweaver777
Copy link

The Svelte version of the useForm helper is incorrectly reading values from the Svelte store by using this.

On line 68 of the setError function is directly referencing this.errors variable to get a list of the current errors to add back into the store along with any new errors. This is not the correct way to get current values from a Svelte store. You need to subscribe to the store or use the store's update function which passes in the stores current values to the callback. In this case when you call clearErrors and then immediately setError the internal variable this.errors still contains the old errors that were cleared in the clearErrors function. The store hasn't updated them yet.

With this PR I updated the setStore function to allow the value parameter to be a callback (in addition to string and object) that passes in the current store values. This allows the setError and clearErrors function to have access to the current store values by using the callback.

This should also fix #1521

@daleweaver777 daleweaver777 changed the title Update useForm to property update/read values in Svelte store Update useForm to properly update/read values in Svelte store May 30, 2023
@daleweaver777 daleweaver777 changed the title Update useForm to properly update/read values in Svelte store Update useForm to correctly update/read values in Svelte store May 30, 2023
@reinink
Copy link
Member

reinink commented Jun 9, 2023

@weavdale Awesome — thanks for this! I'm going to do some testing, and get #1568 updated after merging this in.

@daleweaver777
Copy link
Author

Awesome! Also Svelte 4 will be released in the next week or two. Not supposed to be much breaking changes but you can start testing with the beta. https://github.com/sveltejs/svelte/releases/tag/svelte%404.0.0-next.1

Rich just did a video recently with the changes coming.
https://youtu.be/AOXq89h8saI?t=195

There will be a new website with a migration guide but Svelte 4 is mostly a house cleaning release to prepare for Svelte 5.
https://svelte-dev-2.vercel.app/

@daleweaver777
Copy link
Author

Svelte 4 was released today.
https://svelte.dev/blog/svelte-4

@reinink
Copy link
Member

reinink commented Jun 22, 2023

@weavdale sweet! going to give it a try! 🤞

@daleweaver777
Copy link
Author

Glad you got Svelte 4 support pushed out. Great job!

@reinink
Copy link
Member

reinink commented Jun 30, 2023

With this PR I updated the setStore function to allow the value parameter to be a callback (in addition to string and object) that passes in the current store values. This allows the setError and clearErrors function to have access to the current store values by using the callback.

@weavdale can you explain how you would use this callback?

@daleweaver777
Copy link
Author

daleweaver777 commented Jun 30, 2023

@reinink sure! On line 66 of the current userForm.js you are trying to access a Svelte store's value from inside the Svelte store by calling the variable directly this.errors.

setError(key, value) {
    this.setStore('errors', {
        ...this.errors,     // Can't access a reactive store value this way because it may not be updated yet
        ...(value ? { [key]: value } : key),
    })

    return this
},

You can't access reactive store values in this way because Svelte may have not updated the internal value yet. You can read a store value using the subscribe method, set the value using the set method, or update the value using the update method (https://svelte.dev/docs/svelte-store). update is a method that takes one argument which is a callback. The callback takes the existing store value as its argument and returns the new value to be set to the store.

So in the code above you are trying to spread the existing errors back into the errors variable using ...this.errors. This will not always get the current value of errors. Especially if you update errors and then immediately try to access the value of errors like you are doing on line 137. You clear errors and then assign new ones using the setError function above. You would think that when setErrors function calls ...this.error that it would be empty. But it's not. It still has all the old errors that you just tried to clear. That's because this.error wasn't updated yet by Svelte. So all the cleared errors get added back.

onError: (errors) => {
    this.setStore('processing', false)
    this.setStore('progress', null)
    this.clearErrors().setError(errors) // This will add all cleared errors back into errors variable

    if (options.onError) {
        return options.onError(errors)
    }
},

So I proposed to modify the setStore function to also accept a callback that gets the store's current values passed into it.

setStore(key, value) {
    store.update((store) => {   // current store values gets passed in by the update method
    // If "value" is a callback, pass the current store's value into it so it has access to existing values.
    // Then the callbacks return value is assigned to _value which is then used to update the store 
    // with the new value.
    const _value = typeof value === 'function' ? value(store) : value 
        return Object.assign({}, store, typeof key === 'string' ? { [key]: _value } : key)
    })
},

With this updated setStore function, we can modify the setError function to pass a callback to setStore. That way setStore will pass the current value of the store to setError's callback so it can spread existing errors and add any new ones and then return the value back to setStore to update the store's value.

setError(key, value) {
    this.setStore('errors', (store) => ({
        ...store.errors,
        ...(value ? { [key]: value } : key),
    })) 

    return this
},

Here is a great little video of building your own custom Svelte store using the store contract. Helped me better understand how Svelte's writable store is working under the hood.
https://www.youtube.com/watch?v=wpog6waQrNo

@daleweaver777
Copy link
Author

Any updates on the progress of fixing useForm for Svelte?

@daleweaver777
Copy link
Author

I'm am closing this PR because I like setStore implementation for useForm that PR #1614 and PR #1610 are proposing. Much cleaner and doesn't require the setStore function to receive a callback to update specific store values.

@daleweaver777 daleweaver777 deleted the fix-svelte-store-usage branch July 26, 2023 14:07
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.

useForm helper on 422 validation errors in the helper won't update
2 participants