Basic implementation of validator-cvapi

[benbender]

Here is the basic implementation. At the moment, I have two main areas which need some more work:

1. The options-interface is two complicated for my taste.
   Atm, it supports defaults per ValidityState-error-key and my "per-control-api".
   It resolves the messages with priority for the "per-control-api" as I consider them to be more specific.
   While implementing the "defaults-api", I realized that it's not guaranteed, that only one error-key is true. So I do a simple find on them and take the first match. I had no clue how to prioritize them further, as I couldn't find any information besides the shape of the state.

So the interface is quite flexible, but it needs some knowledge about the resolution-strategy and may be confusing to new users.

To be honest, I don't like this implementation very much as it forces you implicitly to supply every possible value beforehand or may leak default-messages where this may not the desired outcome. Afaik it is not guaranteed which error-keys in ValidityState are flipped under which circumstances. At the end it may be browser-specific.

That's the reason why I opted for a clearer resolution-strategy in the first place where a returned string/result will definitly be the error-message of the invalid control.

It would be perfectly fine for me if you decide the direction how to go forward with this one.

Example of a perfectly possible config for the actual implementation:

    extend: validator({
      defaults: {
        badInput: "bad is bad!",
        patternMismatch: "The pattern is the key!",
        valueMissing: "Have you seen my value?",
        tooLong: "Size doesn't matter. The inner values count!",
        tooShort: "Size doesn't matter. The inner values count!",
        customError: "Be yourself!",
        rangeOverflow: "The more, the better.",
        rangeUnderflow: "Too little, too late.",
        stepMismatch: "Every step you take... Every move you make...",
        typeMismatch: "Typescript will yell at you too! Be warned!",
      },
      controls: {
        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!';
        },
      }
    }),

2. testing.
   I'm quite new to svelte and I'm a bit unsure how to test this one as this is a library on one hand where I would normally test its surface and dependent on the DOM on the other, where I would test the DOM normally. This gets more confusing to me as in svelte both domains are more intermixed as I'm used to in the react-world. Do you have any advice on this?

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pberganza10/felte-docs/Bm6MEgUUUD2xcQxWpT1TbERTyyjF
✅ Preview: Canceled

[benbender]

@pablo-abc I just realized that I had to choose "draft pull request" beforehand. As you may notice, I'm not that familiar with github. Should I close this and reopen as draft?

[codecov-commenter]

Codecov Report

Merging #48 (196af46) into main (c8b1808) will decrease coverage by 0.63%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
- Coverage   99.32%   98.68%   -0.64%     
==========================================
  Files          38       39       +1     
  Lines         885      915      +30     
  Branches      282      292      +10     
==========================================
+ Hits          879      903      +24     
- Misses          5        9       +4     
- Partials        1        3       +2     

Impacted Files	Coverage Δ
packages/validator-cvapi/src/index.ts	80.00% <80.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8b1808...196af46. Read the comment docs.

[pablo-abc]

Just transformed it to a draft! No worries about it. Give me a while to look over the rest!

[pablo-abc]

Going further into why I'm proposing an API that uses the validity states as key rather than the fields:

1. For most use-cases, the validation message does not need to know what field it belongs to (since in most cases it's always somehow next to the input itself). So validation messages always end up having an identical format for each type. E.g. "This field is required" will be next to, or close to the field it's referring to. Knowing that it refers to (for example) the password field itself is irrelevant most of the times. No need for the message to be as specific as to say "The password field is required". Providing only per-field messages would require quite a lot of duplication in most cases.
2. In most cases, custom messages rely more on the current value rather than what it belongs to. E.g. "${value} needs to be at least ${rule} characters long". And these customisations are generally minimal.
3. Felte assumes fields can be deeply nested. And it imposes no restriction on how nested it can be. In these cases, setting per-field messages would require a lot more code:
   For example, data can look like:

{
  account: {
    profile: {
      name: '',
      lastName: '',
    },
    preferences: {
      publicEmail: false,
    },
  }
}

4. ValidityStates are a discrete number of possibilities. Which would be easier to type and manage within the validator.

---

More or less what I had in mind was to either accept a string or a function, which would give quite a bit of flexibility while keeping the code the developer has to write quite concise.

Something like this:

{
    required: 'This field is required',
    tooShort: ({ value, rule }) => `${value} is needs to be at least ${rule} characters long. It's currently ${value.length} characters.`,
}

(Of course the properties passed to it and naming are up for debate)

This API would potentially allow for per-field messages in edge cases.

{
    required: ({ name }) => {
      if (name === 'password') return 'You probably want a password for your account';
      return 'This field is required';
    },
}

---

Needing per-field messages as the main option seems to favor edge-cases rather than most common cases of validation reporting. And of course we'd assume that if no value is provided, the default browser's message would be used.

[pablo-abc]

So we are going by levels of possible edge cases.

1. The default behaviour, which would be the path with less friction, would be a simple call to validator() without configuration that would use the browser's messages.
2. Then maybe you'd like to actually have a specific default message for a rule, on which the per-rule API would come into play.
3. Then for some rules you might want to have some dynamic values, where the function form of this API would come into play.
4. Finally a specific field might need a "special" validation message, for which the function form can still handle it.

I hope I'm making sense right now, to be honest 😅😅

[pablo-abc]

Regarding testing:

Besides reporter-svelte everything is tested as VanillaJS with DOM testing library in some cases. Felte's core is quite framework agnostic since the form action only needs to receive a native HTMLFormElement. In fact with the rest of the validators you'll notice I'm not even using the DOM at all. Since this package does require the DOM, the only difference would be that we'd need to create a form element and pass it to the form action like form(formElement).

Of course, I wouldn't worry about this right now. And you're doing a tremendous amount of work already (I can't thank you enough!). When a proper an API has been established I'd be happy to do the testing.

---

Btw, I've added you as a contributor in the README. Following the all-contributors spec I should have added you a long time ago, so I sincerely apologise for the delay.

[benbender]

I'll think I'll have to think about the best solution a little bit more, but just some quick thoughts:

• In some of my latest projects I had many cases, where it wasn't enough to tell people that a field (f.e.) is generally required, but why it is required. So in those cases, context mattered very much...
• Afaik things like "rule" aren't clearly exposed by the given api. If we want this kind of functionality, we would need to implement it ourselves.

And some more general ones:

• If we want this whole topic of error-messages to be much more flexible and sophisticated, maybe the individual validator is the wrong place to do it? Maybe all those rules, options and cases should be handled at a higher level with a common api and only the default-messages should be the concern of the individual validator?
• same about field-nesting: basically thats dx-sugar by felte and thats fine. HTML-forms don't really have nesting (besides array-values for multi-selects/checkboxes/etc). So if nesting is a supported general case for the configuration (to have nice state-handling in svelte), maybe it should be handled in the core and felte should expose both, the nested object and the dot-notation, to the individual validator. That would be quite useful here, because this concrete validator would need to reverse the process of parsing the nested structure into the dot-notation anyways as there is no such thing as nested fields in the dom we are accessing here.

[pablo-abc]

I understand your concern here. But as far as I can see this can still be considered an edge case. Even when context is needed, most of the times it has more to do with what the input contains rather than which input it is.

While the default behaviour (browser messages) would remain the same; for simple validation requirements with generic validation messages, with a per-field API we are asking the developer to go with something like this:

{
  field1: 'This field is required',
  field2: 'This field is required',
  field3: 'This field is required',
  field4: 'This field is required',
  // ...
}

And this can start to look even more repetitive with more validation added:

{
  field1: (state) => {
    if (state.tooShort) return 'The value you typed is too short';
    if (state.valueMissing) return 'This field is required';
    return 'Invalid value';
  },
  field2: (state) => {
    if (state.tooShort) return 'The value you typed is too short';
    if (state.valueMissing) return 'This field is required';
    return 'Invalid value';
  },
  field3: (state) => {
    if (state.tooShort) return 'The value you typed is too short';
    if (state.valueMissing) return 'This field is required';
    return 'Invalid value';
  },
  field4: (state) => {
    if (state.tooShort) return 'The value you typed is too short';
    if (state.valueMissing) return 'This field is required';
    return 'Invalid value';
  },
  // ...
}

A per-field API would require generic validation messages to be repeated for each field that uses it in the form.

[pablo-abc]

The reason the validation messages functionality is not higher up is that the core is completely decoupled to what validation strategy you're using. Third-party libraries each one have their own methods for setting custom validation messages. E.g. with yup you can use a top level locale dictionary or setting a per-rule validation message on the schema.

This is why a validator extender's only responsibility is to grab whatever the validation strategy you desire to use returns when validating and transform it to what Felte expects (basically an adapter). In this case this validator can be seen as an "adapter" between the Browser's validation strategy and Felte. As long as this validator returns something that Felte knows how to handle, it is ok.

In a more general sense, an extender's purpose is to translate any behaviour you desire back into something Felte can work with.

---

For your second point I do agree that the getPath method is not so discoverable. I do feel this can be solved somehow by documenting the common package, and some more information can be passed down to the extenders. Although completely removing the potential need for the common package seems quite hard.

Anyway I'm definitely up for discussing this more. Ultimately, improving Felte into providing the best DX/UX is the goal and any changes that need to be done to achieve that are never out of the questions (which is why I have kept Felte's version pre 1.0.0 until more feedback is received).

[benbender]

Nesting/General responsibilities:
The problem I saw, but couldn't grab, is simple: I don't think that felte supports nesting or has assumptions about the structure of the data. It claims in the docs, but the implementation is in the validators and not in core. If it would have, there should be implementable interfaces, and much more of the lifting, inside the core.
What felte imho actually has, is some support-infrastructure for more complicated cases handled by some of the validation-libs. Thats fine, but no contract to other packages. This specific validator doesn't have any support for those more complicated cases as it basically only mirrors the dom. So if we want to support those complicated cases, we have to emulate them. I would consider this as unnecessary bloat as it yields no real benefits.

So I would opt for either a) don't support nested states/configs in this package (my preferable option) or b) lift up the infrastructure and provide clear interfaces which should be implemented by every validator.

Config/error-messages
For the error-messages I would propose a simplified config - following the arguments above:

export type ValidatorConfig = {
  defaults: (state: ValidityState, control: FormControl) => string,
  [path: string]: (state: ValidityState, control: FormControl) => string 
};

And this simple implementation:

// check if there is an error-message for that control in the supplied config
// either for the specific path or as a default
if (options?.[path] || options?.defaults) {
    cvErrors[path] = (options?.[path] || options?.defaults)(control.validity, control);
    // if no supplied error-msg can be found, fall back to the browser-supplied default
} else {
    cvErrors[path] = control.validationMessage;
}

The reasoning behind that interface and implementation follows three thoughts:

• It should be predictable, simple, clean: Calls a given function returning a string. Use that string as error-message.
• It should be flexible: The callback gets the control as a second argument, enabling cases such as "Min x chars, only y given".
• The implementation shouldn't be more complicated than the complete package ;)

Testing
I'll think I'll follow the approach of reporter-cvapi if you don't advice against it.

[pablo-abc]

I'd argue with your first point: Felte does support nesting in its core. Using named fieldset elements or naming such as name="account.username" does create a nested data structure within Felte's core (since it tends to be a common use case for complex forms). If the user decides to use these features, your extender will receive data/errors/touched stores with nested properties and the contracts with these packages should be them keeping that same structure. If you were making your own extender for your own internal use, I'd agree that going out of your way to support Felte features that you will not use is not necessary at all. But if we are planning to add a package to our core @felte/* packages then it's only reasonable for them to adhere to the same contract.

Currently in our common package we do export lodash-like utility functions to handle these scenarios: _get, _update, _set (originally named for only internal use as you can see by the underscore in the name. I realise this should change).

---

The function argument you pass seems perfect to me! In fact, with that signature alone ((state: ValidityState, control: FormControl) => string) you can even set per-field messages (since you're getting the control within it. Which would let you get rules set, field name, etc).

From what I've established previously, if we want to adhere to the contract our other core packages have with Felte, having a per-field configuration would imply quite a bit added complexity to this package's implementation.

In general I still don't see the benefit of having a per-field configuration (+ the complexity of implementing it) when a function argument provides the flexibility for that as an edge case. (either if we do a single function argument as proposed in your message or if we use a configuration based on validityStates).

---

As a side note, main already passes an addValidator method to extenders. It's still not published but you should be able to test with it while working within the monorepo.

I apologise for not answering earlier. This week has been quite hard on balancing OSS + work time 😅.

[benbender]

I'd argue with your first point: Felte does support nesting in its core.

The question here is basically how "support" is defined. But you are right in the sense that felte accepts nested inputs in its config and can handle them generally. That shouldn't change.
As the dotted-notation is needed anyways to map to the HTMLElements in the Dom, the solution might simply be to do the parsing of a nested config earlier in the process and offer both notations to validators/extenders.
Maybe this would require to do the parsing of a possibly nested config into the dotted notation a bit earlier in the process in felte but as it has to be done anyways I don't see any negative effects? With this solution validators/extenders could flexible optin to exactly the notation they need/can handle without any overhead in computation or size.

What do you think?

As a side note, main already passes an addValidator method to extenders. It's still not published but you should be able to test with it while working within the monorepo.

\o/

I apologise for not answering earlier. This week has been quite hard on balancing OSS + work time 😅.

This time, I'm a bit in a hurry. So no apologies necessary. Everything's fine!