Add strict option

[AndreasHald]

Hi @ciscoheat based on the discussion here Alex and I have attempted to add a strict option to superValidate

The attempt was to get superValidate to return errors if the input does not satisfy the provided schema. The heavy lifting is still done by Zod this PR just removes default values when strict=true so that the input is parsed directly.

We've added several tests which is probably the best way to examine our intention.

I tried to find the documentation, so I could add that as part of this PR - but I could not find it? is it in another project?

[ciscoheat]

Thank you! I'll check this out locally soon, just added a comment. The documentation is here: https://github.com/ciscoheat/superforms-web

[ciscoheat]

Thank you for doing this, I think the feature is useful, but there's one difficult problem, which is that data must adhere to the schema output (inferred) type. If the schema is:

z.object({
  foo: z.string() // string, not string | undefined
})

Then data cannot be {} as in the tests. There are some options to consider:

• Set valid to false when strict is true, but coerce the failed fields to their default values.
• Throw an exception when strict is true and unknown keys are found, like Zod does it. Puts a larger burden on the library consumer.

Any ideas? I'll make some updates to the PR meanwhile.

[AndreasHald]

You are very welcome :)

I think my clear preference would be to throw an exception. Basically "enforce" that the consumer provides the shape of data that the endpoint requires. It also makes sense from a debugging perspective ("oh if failed let me inspect the input data to see what's missing" coercing would remove that) I agree that it put's a larger burden on the consumer, however as it is opt-in I think that would be fine?

It also gives the ability to integration test your endpoints since you can test that a consumer providing nonsense will be rejected.

Do you have any outstanding work on the branch? or should I attempt to rewrite it to fail instead?

[alexbjorlig]

Just adding one thing.

Would be really great to have something like Playwright component test to capture/describe/test this behaviour (that we eventually decide on)

[ciscoheat]

Sounds good! There's a SuperFormError error class that you can use for that here.

I've made a PR to your PR, if you can merge that then it's up to speed: 21RISK#1
Can you also add me as a collaborator to that branch? There should be a checkbox for that at the bottom.

Also, it would be interesting to hear your thoughts about what to do in non-strict mode when invalid data is posted. The scenario I have in mind is this:

const schema = z.object({
  len: z.string().transform((val) => val.length).pipe(z.number().min(5));
})

type T = z.infer<typeof schema>

Now if len: "four" is posted as FormData, it will fail, and NaN will be the content of data.len, but for the best UX, the input name="len" type="text" field should stay the same, though that won't be possible without breaking type-safety, since data.len must be a number. Here's where the unified input/output schema breaks down for Superforms. A possible idea for v2 is to return a different type when validation fails:

{
  valid: true,
  data: T,
  // ...
} | {
  valid: false,
  data: Partial<T>,
  // ...
}

And only populate the correct data fields, if valid is false. Then if a field doesn't exist in data, it won't be updated on the client. But that makes for a worse DX in the events on the client, since then you cannot assume that any data exists there. Which makes me hesitant to do anything about it, and just leave it is an edge case. Any ideas?

[AndreasHald]

Hi @ciscoheat - while working on the branch, I've had some insights into why you initially built it in the way you did. And I have shifted my opinion 180 Towards not throwing an error but instead having valid:false if the input object does not live up to the schema.

This makes it easier to implement your own error handling in load functions if valid:false but still gives the flexibility of having the form be invalid if someone posts nonsense to your form actions.

My implementation here is straight forward, When strict is true we parse the input data 2 times, with defaults and without defaults - This way we can in the strict case do our schema parsing on the data without defaults, and still return the data with sensible defaults

In this way the output data will always adhere to the schema using the defaults you have created, however the parsing of errors and valid will in strict mode happen on an object without these defaults.

For example in this test case

{
      name: 'Should be invalid if foo is not present in object and strict=true',
      schema: z.object({
        foo: z.string()
      }),
      input: {},
      data: {
        foo: ''
      },
      valid: false,
      errors: {
        foo: ['Required']
      },
 },

The input data is an empty object, so the defaults are applied to the output object, however because strict is true, valid is false, and errors indicate that foo was missing in the input.

One thing to note is, that to my eyes currently the strict=false scenario does not implement defaults if the input is a POJO is this intentional? I've created a small block for this is strict=true to not break anything but should this be the default everywhere?

I would have preferred to break the flow of superValidate into more independent blocks to make the code more readable, but have deferred to your implementation as to keep this PR on topic. But how would you feel about a different PR on this?

[ciscoheat]

I've added tests for non-strict mode as well. Can you review the src/strict.test.ts file to see if everything is as you expect it to behave in strict mode?

[alexbjorlig]

I think the changes look solid.

In the near future we could always dicuss if it would make sense to add some "debugging" meta on the return-type, but that's out of scope for now.

I will test it against our internal integration tests in 21RISK, and hopefully everything will be green 😎

@AndreasHald was not feeling well yesterday - so let's check with him when he's back

[ciscoheat]

Hi @ciscoheat - while working on the branch, I've had some insights into why you initially built it in the way you did. And I have shifted my opinion 180 Towards not throwing an error but instead having valid:false if the input object does not live up to the schema.

This makes it easier to implement your own error handling in load functions if valid:false but still gives the flexibility of having the form be invalid if someone posts nonsense to your form actions.

Yes, this was my reasoning for doing that as well. It's complicated to reconstruct the whole SuperValidated structure if an exception is thrown and you're left with nothing.

My implementation here is straight forward, When strict is true we parse the input data 2 times, with defaults and without defaults - This way we can in the strict case do our schema parsing on the data without defaults, and still return the data with sensible defaults

I updated the code to create both of these in the same run.

The input data is an empty object, so the defaults are applied to the output object, however because strict is true, valid is false, and errors indicate that foo was missing in the input.

Sounds good, that should give us the best of both worlds.

One thing to note is, that to my eyes currently the strict=false scenario does not implement defaults if the input is a POJO is this intentional? I've created a small block for this is strict=true to not break anything but should this be the default everywhere?

Yes, thinking back, I decided earlier that any POJO sent to superValidate should basically be parsed in what we now call strict mode. So the thing back then was more about parsing FormData/UrlParams or not. I think it's fine like that for v1 (don't want to introduce any breaking changes), but for v2 it may be better to rely solely on the strict mode, allowing partial POJO's as well in non-strict mode. But I'm not certain, can you see any potential problems with that?

I would have preferred to break the flow of superValidate into more independent blocks to make the code more readable, but have deferred to your implementation as to keep this PR on topic. But how would you feel about a different PR on this?

The superValidate function is partitioned into a few functions, and I don't like to break out coupled code just for the sake of it, especially if it's not going to be reused. It will be rewritten in v2 anyway, so I see no need. But thank you for asking, of course!

[AndreasHald]

I'm back :) - this looks great!

I updated the code to create both of these in the same run.

Much better.

Yes, thinking back, I decided earlier that any POJO sent to superValidate should basically be parsed in what we now call strict mode. So the thing back then was more about parsing FormData/UrlParams or not. I think it's fine like that for v1 (don't want to introduce any breaking changes), but for v2 it may be better to rely solely on the strict mode, allowing partial POJO's as well in non-strict mode. But I'm not certain, can you see any potential problems with that?

Ah I see, yeah it's probably best not to introduce such a breaking change until v2, I was simply wondering. Personally I'm a big advocate of consistency, so I would argue the behaviour would be "better" if both flows (POJO and FormData) behave the same, And it's simply a matter of representing data as either FormData UrlSearchParams or POJO - however I can see consumers being confused that the data they pass in Is changed when they do superValidate in a load function. it could probably be argued both ways.

The superValidate function is partitioned into a few functions, and I don't like to break out coupled code just for the sake of it, especially if it's not going to be reused. It will be rewritten in v2 anyway, so I see no need. But thank you for asking, of course!

All good :)

We've tested this branch against our internal integration tests and everything looks good - so on my end we are clear to merge 👍

[ciscoheat]

Nice, will merge soon and release a new version within a week or so, together with some other fixes!

[alexbjorlig]

I think you should wait before merging this.

There is an issue with superValidate and no-data, i.e. when creating a form, we did not create a test for this.

When calling superValidate with {strict: true} and no data, errors are not shown in the form.

I think this is a rather bad API design, I would strongly expect {strict: true} to show the errors in the form. This potentially conflicts with the errors flag.

A solution could be to ignore the error flag when strict is true.

[ciscoheat]

With no data at all, errors won't be displayed, but it would make sense to make the errors flag default to true in strict mode. That makes it flexible for the load function.

[alexbjorlig]

With no data at all, errors won't be displayed, but it would make sense to make the errors flag default to true in strict mode. That makes it flexible for the load function.

That sounds like a great solution. Did you ever think about having 2 functions, one dedicated to the load function and one function dedicated to the form-action?

@AndreasHald and I have used a considerable amount of time understanding how different the function calls to superValidate right now are in the 2 scenarios. For example; when would you ever call superValidate in the formAction without passing in the request? Maybe it could be an upgrade for version 2 or even 3 🤔

[ciscoheat]

That sounds like a great solution. Did you ever think about having 2 functions, one dedicated to the load function and one function dedicated to the form-action?

I have, but the difference would be so small that it's better to keep the API simple.

For example; when would you ever call superValidate in the formAction without passing in the request? Maybe it could be an upgrade for version 2 or even 3 🤔

You can parse FormData, extracted from the request, or the url params in the posted request there as well.

[alexbjorlig]

You can parse FormData, extracted from the request, or the url params in the posted request there as well.

Sure, but there is no scenario where you don't want to parse in data. I would argue that the API today is almost too simple, as it does not guide the developer on what arguments to pass in. But again, it's another discussion 🤔

[ciscoheat]

I will have a fix added soon.

[ciscoheat]

Nice catch @alexbjorlig, I like how this turned out! Will be very useful for v2 as well.