Is it a good practise to rely on DOM to get form field value ?

[rccc]

I would like to open a discussion.

I tried to manage dynamic group form with these initial values :

{
	userGroup:"admin",
	users: [{
	    "id": 1,
             "name": "Leanne Graham",
             "username": "Bret",
	      "email": "[email protected]",
	},
	{
	     "id": 2,
	     "name": "Ervin Howell",
	     "username": "Antonette",
             "email": "[email protected]",
       }],
       activated:true
}

I find tricky to match nested field value to their reference in stores, we have to set the name accordingly and explicitely, and we have to add on;change event binfing to each form fields.

I was wondering if the handleChange function could not be avoid as we already have the binded modified values in $form. We could rely on $form subscription to catch modified values and validate them :

       import { diff } from 'deep-object-diff';

	const form = writable(initialValues);
       	let mirror = JSON.parse(JSON.stringify(initialValues))
       ...
	form.subscribe(newValue => {
		let updates = diff(mirror, newValue)
		if(updates && Object.keys(updates).length > 0){
			mirror = _deepCopy(newValue)
			traverseUpdates(updates)	
		}
	})
        ...

I tried to go further and here is a quick POC using a diff to match updates from $form.

import { diff } from 'deep-object-diff';
import { derived, writable} from 'svelte/store';

export const createForm = (config) => {

	const initialValues = config.initialValues || {};
    const validationSchema = config.validationSchema;
  	const validateFunction = config.validate;
  	const onSubmit = config.onSubmit;

	const form = writable(initialValues);
	const errors = writable({});
	const touched = writable({});

	let mirror = JSON.parse(JSON.stringify(initialValues))

	form.subscribe(newValue => {
		let updates = diff(mirror, newValue)
		if(updates && Object.keys(updates).length > 0){
			mirror = _deepCopy(newValue)
			traverseUpdates(updates)	
		}
	})

	const isValid = derived(errors, ($errors) => {
		return Object.keys($errors).length > 0
	});

	function traverseUpdates(obj) {
		let arr = [] 
		function doTraverse(obj){
			for (let k in obj) {
				if (obj[k] && typeof obj[k] === 'object') {
					arr.push(k)
				  	doTraverse(obj[k])
				} else {	
					let path = ''
					arr.push(k)
					path = arr.join('.')
					touched.update((o)=> {
						_set(o, path, true)
						return o
					})
				  	validateFieldValue(path, obj[k])
				}
			}
		}
		doTraverse(obj)
	}

 	function validateFieldValue(path, value) {


	    if (validateFunction) {
	    	// isValidating.set(true);
	      	return Promise.resolve()
	        	.then(() => {
	        		let field = path.includes('.') ? path.split('.').slice(-1)[0] : path
	        		return validateFunction({[field]: value})
	        	})
	        	.then((errs) =>{
	        		if(errs && Object.keys(errs).length > 0){
	        			errors.update((o)=> {
	        				for (let k in errs){
	        					console.log('err',errs[k])
	        					_set(o, path, errs[k])	        				
	        				}
	        				return o
	        			})
	        		}
	    		})
	        	.finally(() => {
	          	// ifisValidating.set(false);
	        	})
	        ;
	    }

	    return Promise.resolve();
	}

	function _deepCopy(input){
		return JSON.parse(JSON.stringify(input))
	}

	const _set = (obj, path, value) => {
		// Regex explained: https://regexr.com/58j0k
		const pathArray = Array.isArray(path) ? path : path.match(/([^[.\]])+/g)
		pathArray.reduce((acc, key, i) => {
			if (acc[key] === undefined) acc[key] = {}
			if (i === pathArray.length - 1) acc[key] = value
				return acc[key]
		}, obj)
	}

  return {
    form,
    errors,
    touched,
    isValid
  };
};

Here the validatiion function is able to update errors store while keeping the initial values structure. No needs to use handleChange function, binding data seems enough.

Maybe this is not the best way to handle form updates, this is just a discussion, comments are welcome.

[larrybotha]

@rccc thanks for the issue. It seems that matching validation to nested properties is currently not a good experience, but I don't yet have a clear understanding of the issue.

Please create a CodeSandbox that demonstrates the issue; you can use the following template: https://codesandbox.io/s/react-forms-lib-bug-report-esdl2?file=/App.svelte - it'll help make the issue much clearer.

Also, if you're using bind:value={$form.someProp} then you may not need on:change={handleChange}.

[rccc]

Hello, this is not an issue, it is just a discussion about getting updated value from the DOM node. Look at this :

  function handleChange(event) {
    const element = event.target;
    const field = element.name || element.id;
    const value = isCheckbox(element) ? element.checked : element.value;

    return updateValidateField(field, value);
  }

No one realized that we get the data from the DOM node while the updated value is already in the $form store ?!

I do not think it is a good idea and i think that those who use this library should not need to use the "onChange" method to get the value from the DOM element.

Currently, in this library, if you use bind:value={$form.someProp}, you still need to use on:change={handleChange} to get the updated value, as i said, this library rely on the DOM node value attribute.

Sorry for the confusion, if no one want to discuss about this point then you can close this issue. I have my own micro library based on this one.

Eric

[larrybotha]

@rccc ok cool, yes, so instead of relying on DOM values we could instead rely on Svelte's store subscription to make updates to values.

I see how it'd be an improvement to rather use Svelte's internal stores to manage values (the handleChange helper would have to be maintained in order to prevent a major version bump), and a PR for that would be appreciated, but before we go down that route, can you provide an example in CodeSandbox that demonstrates what you said here:

I find tricky to match nested field value to their reference in stores, we have to set the name accordingly and explicitely, and we have to add on;change event binfing to each form fields.

I've put together a demo using your values that only uses bind:value without on:change and name here:

https://codesandbox.io/s/priceless-tereshkova-c9lnd?file=/App.svelte

Is there anything you could add to that demo to show what you're finding tricky?

[rccc]

@larrybotha

Many thanks for response and demo !

I looked at you demo and update it with this :

onSubmit: values => {
  alert(JSON.stringify(values));
  console.log('touched', $touched)
}

Please do the same and update one of the email with a new value and submit the form, you will then realize that the "touched" store is still the same ! All properties are still set to false, because from the handleChange and as you use the DOM, you have no way to match a nested property inside the "touched" store or even the "errors" store.

You could do the same to check errors store, same problems, no way to update nested properties !

Take a look at the source code of "handleChange" and you will understand.

You have to set "name" attribute with awful values like "users.[i].email" so that the "set" method could works and set the nested properties values (yes the set method works with "path" notion like in the underscore lib, see https://underscorejs.org/#toPath).

Many many thanks for you attention !
Eric

[rccc]

Please refer to my first post to see what a PR could contains...

The main part is

form.subscribe(newValue => {
	let updates = diff(mirror, newValue)
	if(updates && Object.keys(updates).length > 0){
		mirror = _deepCopy(newValue)
		traverseUpdates(updates)	
	}
})

[larrybotha]

@rccc I see - the $errors store not updating when handleChange is omitted makes everything clear now.

Ok, so this is sort of a bug / feature request: An update to a form value should update all related stores, without relying on handleChange.

I've opened a new issue here: #110

Feel free to open a PR, @rccc!