beynar / kit-query-params

Another easy way to read and write from query parameters in sveltekit and svelte 5.
MIT License
15 stars 0 forks source link

zod for validation #1

Open tcurdt opened 1 month ago

tcurdt commented 1 month ago

Any thoughts on using zod for the schema validation?

beynar commented 1 month ago

Hey ! thank for reaching out.

I think Zod or any other validation library will be overkill and will add some unnecessary complexity to the library. Also, one of the goal here is to remain dependency free and as lightweight as possible.

Beside that, queryParams are outside of the developer realm and may, in many case, be invalid. One of the library responsibility is not only to make sure that state respect and reflect the schema you define but also coerce raw query params values into their corresponding primitive types.

AFAIK, Zod can only coerce primitives values like this library does. So maybe I'm missing something but I do not know what Zod would bring to the table.

tcurdt commented 1 month ago

I think Zod or any other validation library will be overkill and will add some unnecessary complexity to the library. Also, one of the goal here is to remain dependency free and as lightweight as possible.

Instead of adding a dependency, the idea would rather be to separate the state handling from the schema validation.

Beside that, queryParams are outside of the developer realm and may, in many case, be invalid. One of the library responsibility is not only to make sure that state respect and reflect the schema you define but also coerce raw query params values into their corresponding primitive types.

Not sure I follow what you are trying to say.

AFAIK, Zod can only coerce primitives values like this library does. So maybe I'm missing something but I do not know what Zod would bring to the table.

Zod can do more than to coerce to primitives. And I think it's a good idea to keep using one type of validation in a project.

beynar commented 1 month ago

Zod can do more than to coerce to primitives. And I think it's a good idea to keep using one type of validation in a project.

But coercion is the key feature here and we need it right Let's consider the following schema:

const schema = z.object({
  a: z.number(),    
  b: z.boolean()
});

If search is a=123&b=true then a will of type string | null and b will be of type string | null.

From the schema perspective, as long as you define something other than a string, any search param will be invalid. Therefore, we need a coercion mechanism.

Also, users can easily pass invalid types or other keys, remove keys, or add keys to the search params, which will make the schema input invalid. Therefore, we need to traverse the schema, find only the relevant keys inside the search params, and coerce the values. The library does this validation + coercion at the same time and recursively.

I do not say that it will be impossible to do with Zod, but I do not see any value here. Also, Zod does not provide a coercion mechanism for enums, object and arrays but the library does accept enums, arrays, or objects. So using Zod will be some sort of a regression.

Let's consider a nested schema example:

const schema = z.object({
    a: z.object({
            b: z.array(z.string()),  
            d: z.string()      
    })  
})

If search params are empty, the result will be undefined. But from the developer's perspective, 'a' should be an object with 'b' as an empty array and 'd' as null.

I do not see any way to do this with Zod.

But again, I may be missing something about Zod, so please tell me what it will bring to the table that is not possible without it.

tcurdt commented 1 month ago

Just a quick reply. I didn't test it as is, but this should work:

z.object({
  num: z.coerce.number().default(10),   
  bool: z.coerce.boolean().default(false),
  emails: z.array(z.string().email('invalid email address')).default([]),
  some: z.enum(["a", "b"]).default("a")
});

https://zod-playground.vercel.app/

I am still not sure I am seeing your point.

beynar commented 1 month ago

Hum I guess you're right. Let me cook something 👍

beynar commented 1 month ago

Ok i have something working on the zod branch. It will be nice if you can give your thought on it.

Honestly I still kind of think that a zod schema here is overkill and lower the developer experience because a wrongly defined can throw error and also because in order to have the same result the schema has to be much more complex:

const zodSchema = z.object({
        num: z.coerce.number().nullable().catch(null),
        bool: z.coerce.boolean().nullable().catch(null),
        emails: z.array(z.string()).catch([]),
        enum: z.enum(['a', 'b']).nullable().catch(null),
        nested: z.object({
            array: z
                .array(
                    z.object({
                        string: z.string().nullable()
                    })
                )
                .catch([])
        })
    });

const normalSchema = {
        num: 'number',
        bool: 'boolan',
        emails: ['string'],
        enum: '<a,b>',
        nested: {
            array: [
                {
                    string: 'string'
                }
            ]
        }
    };

But I have to admit that zod bring some other features that are nice to have.

tcurdt commented 1 month ago

Not sure I follow the "wrongly defined can throw error". And I would not use catch(null). I would also always provide a default over nullable.

The main selling point is that developers don't need to learn yet another schema language (that is use nowhere else). It might be more concise - but it's also less expressive.

My bottom line is: you are mixing two concern here. One is validation, one is url state. It would be nice to separate them and have the developer bring their own validation.

beynar commented 1 month ago

Not sure I follow the "wrongly defined can throw error".

Yes I won't throw an error per se (thought i doubt that my proxy can handle undefined object) but mean that if i have the following schema:

const schema = z.object({
    enum: z.enum(["a","b"]),
    object: z.object({
         string: z.string()
    })
})

And a query parameter like ?enum=c the input is {enum:"c"} and the result of schema.safeParse() will be undefined. Therefore the state returned by the searchParams function will be undefined and despite what the schema output type will suggest, you won't be able to acesss state.string to read or mutate it. (Thought it might be possible with the current proxy implementation).

By adding the catch everywhere (and my custom validation) I try to preserve the structure of the schema to allow any nested mutation and facilitate the management of the query params.

But maybe it's fine to you to have an undefined state and maybe it's our main disagreement here.

The main selling point is that developers don't need to learn yet another schema language (that is use nowhere else).

That I understand

My bottom line is: you are mixing two concern here. One is validation, one is url state.

Mine is that the goal of this library is to tied them together so that they seems the same things and that the state is as structurally similar to the schema as possible.

tcurdt commented 1 month ago

Yes I won't throw an error per se (thought i doubt that my proxy can handle undefined object) but mean that if i have the following schema:

const schema = z.object({
    enum: z.enum(["a","b"]),
    object: z.object({
         string: z.string()
    })
})

And a query parameter like ?enum=c the input is {enum:"c"} and the result of schema.safeParse() will be undefined.

What do you mean by undefined? That's invalid input and an invalid request. It should give a 400 error. Avoiding such requests is the whole point of that validation schema.

But maybe it's fine to you to have an undefined state and maybe it's our main disagreement here.

Well, I am not sure what else the application is supposed to do when you pass an enum with an invalid value.

My bottom line is: you are mixing two concern here. One is validation, one is url state.

Mine is that the goal of this library is to tied them together so that they seems the same things and that the state is as structurally similar to the schema as possible.

I agree on the latter. I just don't understand why you want them to be so tied together.

Maybe it just means you don't want support other use cases. And that's fine. In the end it's your library.

I think it's cool that you open sourced it. But it might just not be for us then.

beynar commented 1 month ago

What do you mean by undefined? That's invalid input and an invalid request. It should give a 400 error. Avoiding such requests is the whole point of that validation schema.

Ok I think I understand better your point, and our misunderstanding.

The initial goal was to be forgiving with invalid inputs and fallback to a state that has no invalid values but a valid structure.

That because query params are easily editable by user and therefore prone to invalid inputs. Unlike an object you send via a function to an API endpoint that much more in the developer controls.

Also URL are a form of state and can be shared or stored. If you decide to change the schema later all shared pages may returns errors which from a user perspective can be frustrating. It's not like an API call you change inside your code base and your endpoint altogether.

The point of validation library is to validate input because they can break yout app. With a forgiving coercion mechanism it will not. I Try to find a balance here.

But maybe I should add a parameter to disable coercion, it will be true by default with a Zod schema and throws an error on invalid input. Is that your use case ?

tcurdt commented 1 month ago

I guess a middle ground could be to fall back to default values for invalid values. That would make the migration much easier when the schema changes.

I am not entirely sure how easy this works with zod, but it might be worth checking.

Still I think ideally, validation and state management should be two separate things.