Improve extender API

[pablo-abc]

While the current extender API works, it feels quite hacky. A major thing that should be available is to modify or extend the Felte's configuration which currently relies on modifying the configuration object taking advantage of its mutability.

[benbender]

Hey,

as said i #43 I had a quick look at the extender-/validator-API to use the CVAPI for validation and felte for formstate and rendering the error-messages. What I first stumbled over was the fact, that the form isn't available to neither the validate-API nor extender-API (as it was null at my tests).

I'm a bit unsure if the nullified currentform was intentional in the design or a consequence (seemed to be the later as all the other implementations, I had a sneak at, did some sort of null-checking in the extender-api at the beginning).

If you don't want to pass the form as a second argument to validate() for separation-concerns it would be nice to have the form-object at least available to the setup-phase of the extend-API to f.e. add extra eventhandlers for validation-events programmatically.

Reasoning: Atm my use-case, detailed in #43, doesn't seem to be possible by using the felte-apis in the intended way without doing much hackery. But I also have to say that the use-case is clearly an edge-case which might not be worth to lift the barriers introduced by the correct, nice & clear separation of concerns in the APIs...

[pablo-abc]

It is not available to validate for the reasons mentioned previously. While, in the case of the extender API, it is not available during the first call because it happens during component initialisation, not mount. The reason for this is that you might want to set up features during component initialisation (such as context as it's done by reporter-svelte and reporter-solid). This is why you see null checks in some extenders with if (!currentForm.form) return;.

Going back to #43, I understand your use case. There's a few things to consider here:

1. The browser won't even set a forms validity if the form has the novalidate attribute. So checkValidity will always return valid regardless of any attribute on the inputs.
2. If we remove the novalidate attribute then you remove control from Felte completely regarding validation. This will definitely cause conflicts and headaches.

But I do get we want mainly two things here:

1. Minimal repetition of validation (just by using native HTML attributes such as required).
2. Ability to still mix and match validators and reporters.

The main constraint I'd like to keep is to keep our stores (data, errors, touched) as the main source of truth for all things. As an example I can show you the reporter-cvapi which does use setCustomValidity. But the main source of truth for errors is still the errors store.

The flow of data right now looks more or less like this: user input -> data store update -> touched store update -> validation -> error store update -> reporter calls.

An API I think would solve your use case while still adhering to these constraints probably would look like:

<script>
  import { validator } from '@felte/validator-cvapi';

  const { form } = createForm({
    // ...
    extend: [validator({ required: 'This field is required', \* ... *\  })],
    // ...
  });
</script>

<form>
  <input type="email" required />
  <input type="password" minlength={8} required />
  <button type="submit">Submit</button>
</form>

On which the validator would do most of the heavy lifting. But as far as I understand, as long as the form has novalidate, we would need the developer to handle messages and i18n, since we would not have access to the built-in browser ones.

[benbender]

(Update: added a working felte-POC) Ah, I wasn't aware of the fact that the validation isn't running at all if you set novalidate=true on a form (even if it makes perfectly sense...). Nevertheless, I think there might be a better way to achive what I wanted.

There is an invalid-event which can be hooked on to to cancel the "bubble" and read the error-message/validity-state.

So it should be possible to do the following (if I'm not missing out a detail again):

1. set novalidate=true on init
2. Actually not needed. hook on invalid to read the validity-state and validationMessage of an input-element and cancel the event. Use validity-state and validationMessage to update the errors-store of felte.
3. on the validate()-call of felte, set novalidate=false on the form, call checkValidity() on the form (or all inputs, not sure here) read all validationMessage, set novalidate=true afterwards and return the error-messages.

Basically all this does, is defering the call of checkValidity() away from right before the submit-event to a pragmatically user-called place in the form-flow. This should meet all requirements of my use-case and the principles of felte...

Basic POC/example:
(logs correctly and fires the submit-event)

<script lang="ts">
	import type { CurrentForm, Errors, ExtenderHandler, ValidationFunction } from '@felte/common';
	import reporterDom from '@felte/reporter-dom';
	import { createForm } from 'felte';

	// doesnt seem to be neccessary (at least in chrome, firefox, safari on osx)
	// const onInvalid = (event) => {
	// 	// prevent the error-bubble
	// 	event.preventDefault();
	// };

	const validatorCvapiExtender = (
		currentForm: CurrentForm<Record<string, string>>
	): ExtenderHandler<Record<string, string>> => {
		if (currentForm.form && currentForm.config.cvapivalidation !== true) {
			// doesnt seem to be neccessary (at least in chrome, firefox, safari on osx)
			// controls.forEach((control) => {
			// 	control.addEventListener('invalid', onInvalid);
			// });

			const validate = currentForm.config.validate;
			const validateFn: ValidationFunction<Record<string, string>> = () => {
				const cverrors: Errors<Record<string, string>> = {};

				if (currentForm.form && currentForm.controls) {
					// enable native form-validation
					currentForm.form.novalidate = false;

					currentForm.controls.forEach((control) => {
						cverrors[control.name] = control.validationMessage;
					});

					// disable native form-validation, seems to be enough to prevent the error-bubbles on submit
					currentForm.form.novalidate = true;
				}

				return cverrors;
			};

			if (validate && Array.isArray(validate)) {
				currentForm.config.validate = [...validate, validateFn];
			} else if (validate) {
				currentForm.config.validate = [validate, validateFn];
			} else {
				currentForm.config.validate = [validateFn];
			}

			currentForm.config.cvapivalidation = true;

			// doesnt seem to be neccessary (at least in chrome, firefox, safari on osx)
			// return {
			// 	destroy() {
			// 		if (form) {
			// 			controls.forEach((control) => {
			// 				control.removeEventListener('invalid', onInvalid);
			// 			});
			// 		}
			// 	}
			// };
		}

		return {};
	};

	const { form } = createForm({
		extend: [validatorCvapiExtender, reporterDom()],
		onSubmit: async (values) => {
			console.log('onSubmit', values);
		}
	});
</script>

<form use:form id="form">
	<label for="email">Email:</label>
	<input type="email" required name="email" id="email" aria-describedby="email-validation" />
	<div id="email-validation" data-felte-reporter-dom-for="email" />

	<label for="password">Password:</label>
	<input
		type="password"
		minlength={8}
		required
		name="password"
		id="password"
		aria-describedby="password-validation"
	/>
	<div id="password-validation" data-felte-reporter-dom-for="password" />


	<label>
		<input type="checkbox" required name="legal" aria-describedby="legal-validation" />
		Legal foo
		<div id="legal-validation" data-felte-reporter-dom-for="legal" />

		<button type="submit">submit</button>
	</label>

	<button type="submit">submit</button>
</form>

<style>
	input:valid {
		border: 3px solid green;
	}

	input:invalid {
		border: 3px solid red;
	}
</style>

(Best: test-form.svelte-b884fdf4.js, 3.10kb / brotli: 1.13kb for an accessible, internationalized, progressive form-setup. Quite impressive I would say! :)

What do you think about this approach?

[benbender]

PS: to get back to the main-topic of the thread - the addition of a validator-function via extender-api should be handled by felte imho. Checking and combining them seems to be a bit repititive and cumbersome.

Something like this would be nice:

const extender = <Data extends Record<string, string>>(currentForm: CurrentForm<Data>): ExtenderHandler<Data> => {
    currentForm.addValidator(() => {
        // ...
    });

    return {};
};	

[pablo-abc]

That's actually quite ingenuous and much less hacky than I was expecting it to need to be. So as soon as novalidate is set to false the validation errors from the browser are available?

And yeah, as far as I understand the invalid event happens whenever you call checkValidity() on a field, which I thought you'd need to call before checking errors on the field... but it seems the browser sets the errors eagerly. I really like this approach, actually.

A couple of additions I'd think would be necessary are:

• For the sake of flexibility maybe we can let developers override messages by letting them pass an object with the same keys as ValidityState? Since design or business requirements might require specific messaging. Probably we'd need to pass information about the field in order for them to appropriately set the message (e.g. input type and current value).
• Instead of cverrors[control.name] = ..., let's use the getPath util from @felte/common like cverrors[getPath(control)] = ... since it handles possible nested fields.

The way I'm currently making validators that accept configuration is with higher order functions like const extender => (config) => (currentForm) => { \*...*\};

You up for making a PR with this validator? You can copy the files of another validator in the repo and change details accordingly. (It doesn't need to be ready to merge, you may set the PR as a draft).

[pablo-abc]

@benbender you're totally right. Currently modifying the configuration depends on the fact that you can mutate the config object since it's passed by reference. addValidator seems like a good first step towards not relying on that. I was also considering if there would ever need a reason for a dev to override the configuration (for things like modifying onSubmit behaviour`?) or if this would always be purely additive.

[benbender]

That's actually quite ingenuous and much less hacky than I was expecting it to need to be. So as soon as novalidate is set to false the validation errors from the browser are available?

yes

And yeah, as far as I understand the invalid event happens whenever you call checkValidity() on a field, which I thought you'd need to call before checking errors on the field... but it seems the browser sets the errors eagerly. I really like this approach, actually.

me too :) And yes, checkValidity() gets never called because the form is in novalidate=true on the submit-event. Thats why the solution works that simple.

A couple of additions I'd think would be necessary are:

Sure!

• For the sake of flexibility maybe we can let developers override messages by letting them pass an object with the same keys as ValidityState? Since design or business requirements might require specific messaging. Probably we'd need to pass information about the field in order for them to appropriately set the message (e.g. input type and current value).

I'm a bit unsure about this as the config might get quite complicated and leak "native"-error-messages if not done completely. Maybe a better solution would be to offer to configure a callback per field which calls setCustomValidity() for the field on error?

Something like:

interface CvapiValidatorOptions {
  [path: string]: ((state: ValidityState) => string) | string;
}

With this simple implementation:

currentForm.controls.forEach((control) => {
  const path = getPath(control);

  if (options?.[path] && control.validity.valid === false) {
    // TS is yelling at me besides of the type-guard. Not sure why. Somethings fishy with the type-inference.
    if (typeof options[path] === 'function') {
      // @ts-ignore
      cverrors[path] = options[path](control.validity);
    } else {
      cverrors[path] = options[path] as string;
    }
  } else {
    cverrors[path] = control.validationMessage;
  }
});

This would open all possibilities with a quite clear api while being simple to use and we would get type-completion for the state for free.

Config along those lines:

validatorCvapiExtender({
  legal: 'You have to confirm!',

  password: (state) => {
    if (state.tooShort) {
      return 'Yoda once said: Your password strong has to be!';
    }

    if (state.valueMissing) {
      return 'No entry without password!';
    }

    return 'Nice try, buddy!';
  }
}),

• Instead of cverrors[control.name] = ..., let's use the getPath util from @felte/common like cverrors[getPath(control)] = ... since it handles possible nested fields.

Ah, sure. Didn't know about getPath().

The way I'm currently making validators that accept configuration is with higher order functions like const extender => (config) => (currentForm) => { \*...*\};

👍

You up for making a PR with this validator? You can copy the files of another validator in the repo and change details accordingly. (It doesn't need to be ready to merge, you may set the PR as a draft).

Sure, but it may take some days as I have some paid work to do and have been fiddeling around with this for too long this morning :)
And we'll have to see if tests are easily possible as I'm a bit unsure about the validation-api-support in jsdom. And I would like to leave the docs up to you as you know your style and the structures of your docs.

you're totally right. Currently modifying the configuration depends on the fact that you can mutate the config object since it's passed by reference. addValidator seems like a good first step towards not relying on that. I was also considering if there would ever need a reason for a dev to override the configuration (for things like modifying onSubmit behaviour`?) or if this would always be purely additive.

My take here: don't try to overengineer. I think the api is basically fine & easy to understand. Solve problems which are real. Premature optimization is the root of all evil ;)

Btw, the beauty of felte is the fact that it can be quite lean compared to other (react-)solutions because it lets much of the heavy lifting up to the DOM. I would strongly recommend to try to introduce as less bloat as possible (in terms of size and api-surface). As said before, I would like to see the currentForm as second-argument to validate() as it would enable even more direct interaction with the dom. And it would enable the upcoming cvapi-validator to rely only on the validate()-api :)

[benbender]

And, last but not least, a question about the implementation (see comments):

const validatorCvapiExtender = (
    currentForm: CurrentForm<Record<string, string>>
): ExtenderHandler<Record<string, string>> => {
  // Is the "currentForm.config.cvapivalidation"-check neccessary?
  // The extender gets called at least twice and I wanted to ensure to not
  // add it multiple times. Is this neccessary?
  // If it's needed, is there a better place to store this peace of state other
  // than the config-object?
  if (currentForm.form && currentForm.config.cvapivalidation !== true) {
    const validate = currentForm.config.validate;
    const validateFn: ValidationFunction<Record<string, string>> = () => {
       // ...
    };

    // ...

    // see above.
    currentForm.config.cvapivalidation = true;
  }

  return {};
};

[benbender]

First preview of validator-cvapi available here: https://github.com/benbender/felte/tree/feat/validator-cvapi

[pablo-abc]

The reason I'd prefer a single object with each validity state as property is that it's a more common case to have common error messages for all of your validations than a custom error per field. So more most use-cases it'd require little duplication (which I'd also think is better DX?). Similar to how it works in Yup, as an example. So probably accepting either a string or a function that'll receive information about the field that failed would work for better DX in general (unless there's something I'm not seeing?).

---

The thing regarding your cvapivalidation brings another issue with the current extender API. Maybe having defined lifecycles for an extender would make more sense. As of now there's three situations on which extenders are called:

1. On component initialisation (when form == null).
2. On mount (when the form action in use:form gets called).
3. Whenever the form changes (e.g. fields have been added or removed from the DOM).

As of now, no package had to differentiate from mount and update, so I only needed to check for the existence of form to different from initialisation and the rest. So yes, for now that seems to be the only way to differentiate from an update and the first mount call. But it does stay as a note to myself to add a proper way to handle lifecycles.

---

Feel free to make a PR with the current state of the validator as it is right now! You can create it as a "draft pull request" and discuss implementation details over there. I'm open to work in the improved extender API as you work on the validator as we find more inconveniences from the current API 😅

[benbender]

Done in #48 - see you there :)