fabian-hiller / decode-formdata

Decodes complex FormData into a JavaScript object
MIT License
241 stars 7 forks source link

issue parsing optional number #9

Open jdgamble555 opened 9 months ago

jdgamble555 commented 9 months ago

How would I parse an optional number?

const postSchema = object({
  ...
  category_order: optional(number()),
  ...
});

const formData = decode(await request.formData(), {
    numbers: ['category_order']
});

This does not seem to work when optional, as I get this message:

category_order - NaN - Invalid type

when checking the error:

const msg = result.issues[0].path[0].key + ' - ' + result.issues[0].path[0].value + ' - ' + result.issues[0].message;

From this discussion

J

fabian-hiller commented 9 months ago

Thank you for creating this issue. The problem with this issue is that as far as I remember, this library only supports one datatype per field. I will look into it and see if and how we can make it possible to support multiple datatypes.

fabian-hiller commented 9 months ago

I have investigated this case and there is a problem.

const formData = new FormData();
formData.append('key1', null);
formData.append('key2', undefined);
formData.append('key3', 123);
formData.append('key4', "123");
formData.append('key5', true);
formData.append('key6', "true");

Results in:

[
  ["key1", "null"],
  ["key2", "undefined"],
  ["key3", "123"],
  ["key4", "123"],
  ["key5", "true"],
  ["key6", "true"]
]

Therefore, for strings, we cannot distinguish between

This means that for anything that isn't a string, we could try to allow multiple data types by improving data type detection in our source code. But for strings this is not possible.

fabian-hiller commented 9 months ago

Can you send me your FormData entries by executing [...formData.entries()]?

fabian-hiller commented 9 months ago

I changed a few things and released a new version. For non-string types:

Please let me know if this solves your problem.

jdgamble555 commented 9 months ago

Interesting. In my case I'm using SvelteKit with:

await request.formData()

which would be an object. I have a complex schema for a post draft. Basically I am passing:

<input name="category_order" type="number" />

You oddly enough set the value with a string, but the form knows its a number. I want it optional because the post might not have a category where it needs to have an order.

J

jdgamble555 commented 9 months ago

Interesting. Not sure if this is expected, but I get this:

const draftSchema = object({
  category_order: optional(number()),
  ...
});

const formData = decode(await request.formData(), {
    numbers: ['category_order']
});
category_order - null - Invalid type: Expected number but received null

Everything works when not null.

J

Edit: I tried this too, didn't work, same error:

category_order: nullable(optional(number())),
fabian-hiller commented 9 months ago

You can either change your schema to nullable(number()) or nullish(number()) or we can discuss if "" should return undefined instead of null.

Edit: I tried this too, didn't work, same error

Are you sure? Please try it again and send me the error message.

jdgamble555 commented 9 months ago

WIth nullable(number()) I get a warning, but it works:

input.svelte:37 The specified value "undefined" cannot be parsed, or is out of range.

To me optional seems like the obviously answer to work (no matter what you actually translate it to), but as long as there is a set way that is consistent, I have no preference.

J

fabian-hiller commented 9 months ago

Ok. Then I would leave the implementation as it is for now and close this issue.

jdgamble555 commented 9 months ago

But it doesn't work with optional and nullable throws a warning.

fabian-hiller commented 9 months ago

The warning with nullable seems to be related to Svelte and not this library. You should probably pass value ?? '' to your <input /> field to exclude null or undefined as a value.

jdgamble555 commented 9 months ago

Ok, so I put this default value as '' for a number. I get this error with optional(number()):

category_order - null - Invalid type: Expected number but received null

This is how I believe this should work.

If I have optional(number()), with numbers: ['category_order'], I'm explicitly telling decode and valibot that it is NOT a string. optional() should allow for empty strings to be interpreted as undefined here.

As of now, your changes do not fix this problem, as there is not way to declare an optional string.

Technically null should be a disabled input.

J

fabian-hiller commented 9 months ago

optional(number()) results in number | undefined and not in number | ''. Why does nullable(number()) not work for you?

jdgamble555 commented 9 months ago

So correct me if I'm wrong, but I would think it should match the current way valibot works.

If you pass an empty value in a text field:

<input type="text" name="test" />

It will actually just pass an empty string. All inputs are strings and pass an empty string. We differentiate them by their type. However, you don't do:

const schema = object({
  test: nullable(string())
  ...
});

You do:

const schema = object({
  test: optional(string())
  ...
});

I would think you would want it congruent no matter what the type is. Plus, as I said earlier, null actually represents a disabled type. That may not be useful, but worth noting.

Given that is how strings work, shouldn't numbers work the same way? I would think the simple fix for this would be:

decode(await formData, {
    numbers: ['test']
});

Make the test field check for empty strings, and if so convert them to undefined? I'm not sure what you do with string types, but I would think it should work the same way.

Unless I am missing something?

J

fabian-hiller commented 9 months ago

Why do you consider null to be disabled? I would say that null is an explicit missing value and undefined is an implicit missing value.

If you don't like null as the output of your schema, you could use transform to change it to undefined.

jdgamble555 commented 9 months ago

I have not tested this, but I thought a disabled input returns null on the server.

It's not what I like, I just figured you would want a number input to behave the same way as a text input when it comes to optional.

fabian-hiller commented 9 months ago

If I have an empty string as a value for a <input type="date" /> element and call input.valueAsDate, I think it will return null. For a number with input.valueAsNumber it will probably be NaN and for a string with input.value it will be an empty string. We could follow that approach. Are you aware of any other behavior that speaks for undefined when working with numbers?

jdgamble555 commented 9 months ago

I'm not sure what you mean. Those are client functions.

I made a quick test repo: https://github.com/jdgamble555/form-test/blob/master/src/routes/%2Bpage.server.ts

If you run this:

<form method="POST" enctype="application/x-www-form-urlencoded">
    <input  type="number" name="number" />
    <input type="text" name="text" />
    <input type="date" name="date" />
    <input type="file" name="file" />
    <input type="text" disabled />
    <input type="submit" />
</form>

You get these results:

FormData {
  [Symbol(state)]: [
    { name: 'number', value: '' },
    { name: 'text', value: '' },
    { name: 'date', value: '' },
    { name: 'file', value: '' }
  ]
}

Note: Forget about disabled, it looks like it doesn't pass at all.

All input types are passed as strings. The question is, how do you want to validate them so that the validation is consistent when there is no value. I was thinking you would always use optional or you would always use nullable. The way you have it, date and number types use nullable, while text and file, for example, use optional to validate.

This is fine, but my whole point is that maybe they should be translated to undefined instead, since this is how text, file, etc. work so that all of them would use optional instead of some using optional and other types using nullable.

I'm saying maybe it should be consistent across all types when there is no value passed, so I would think using the decode library would tell the parser to be consistent. nullable works fine otherwise.

const schema = object({
    text: optional(string()),
    number: nullable(number()),
    date: nullable(date()),
    file: optional(instance(File))
});
const result = safeParse(schema, decode(data,
    { numbers: ['number'], dates: ['date'], files: ['file'] }
));

J

fabian-hiller commented 9 months ago

Can you explain why optional works for string? As far as I know, an empty string will return an empty string. For files, you might be right, and I agree that we should think about a consistent approach. Can you explain again why you prefer undefined to null?

jdgamble555 commented 9 months ago

So on the client side, you get:

{
    "number": "",
    "text": "",
    "date": "",
    "file": {}
}

Optional works as a string because it is really undefined | string. It is neither null nor undefined, so a string will always pass.

This means both optional(string()) and nullable(string()) work, but you really only need string() alone. You actually use nonOptional() to invoke required.

I did not realize this.

You should not change this, as I would think it should match the frontend. By default, html forms are not required unless you explicitly add required.


So in reality the only problem is number(). This seems to be a frontend and backend problem. From a form perspective, a valid number should really cast like Number('2'). I'm not sure the correct way to handle this. You need to be able to validate an object whose value is a number, and an object whose value is a string cast to a number... assuming it can be casts to a number.

Are you a form validation library or an object validation library? I would think you have to be both since form values would be translated to objects first in most use cases. Another option is to add a separate type like inputNumber().

Either way, a number() fails input, when it shouldn't. This is equally the case on the front and backend with Valibot, so decode won't solve this problem alone.

number - 1 - Invalid type: Expected number but received "1"

J

fabian-hiller commented 9 months ago

You can use valibot and decode-formdata in the frontend and in the backend. It should work the same.

jdgamble555 commented 9 months ago

Ok, so if decode-formdata is the way to accurately handle numbers on both the frontend and backend, then both date and number should behave the same way.

Currently decode turns them both into null, however, only a null date is allowed. It should be allowed for both unless nonOptional() is applied.

J

fabian-hiller commented 9 months ago

Currently decode turns them both into null, however, only a null date is allowed. It should be allowed for both unless nonOptional() is applied.

Can you send me a code example?

jdgamble555 commented 9 months ago

Sorry for confusion. It does show an error for both (I forgot my code only shows first error).

Here is the updated the repo.

But basically:

const schema = object({
    text: string(),
    number: number(),
    date: date(),
    file: instance(File)
});

It will show a null error for number, date, and an undefined error for file. text works fine, as an empty string is still a string.

I guess my thoughts are that they should all be optional and not show an error in any case, unless nonOptional is explicitly stated. This is how forms natively work.

Or maybe just file should be null and not undefined to match date and number. Honestly, I don't know anymore lol.

J

fabian-hiller commented 9 months ago

Valibot is a schema library implemented closely to TypeScript and number in TypeScript does also not allow undefined. Similarly to TypeScript you can create unions with Valibot's union function. nullable, nullish and optional are kind of a shortcut for this.

jdgamble555 commented 9 months ago

number in TypeScript does also not allow undefined

I don't understand what you're meaning here. A number type is obviously separate from undefined or null in TS.

The problem we are talking about is the lack of consistency.

I think what you really need is this:

const result = safeParse(schema, decode(data,
    { strings: ['text'] }
));

That way an empty string is treated as null when coming from a form just like date and number.

Also, you should fix file so that it is null instead of undefined to match how the forms work.

J

fabian-hiller commented 9 months ago

That's a great idea! Thank you! I will think about it.

jdgamble555 commented 5 months ago

@fabian-hiller - Did you think about doing this?

Thanks!

J

fabian-hiller commented 5 months ago

Sorry for not answering. Valibot already takes up all my time.

That way an empty string is treated as null when coming from

But what if someone deliberately wants to pass an empty string?

jdgamble555 commented 5 months ago

I think the question is about required. It covers cases with null, undefined and ''.

I realize Valibot handles this now (since this post was created) with pipe(string(), nonEmpty()), but obviously this is not clean, and you have to manually redo the error message.

You could manually add something like treatStringAsNull: true, but not sure worth it.

fabian-hiller commented 5 months ago

Thanks for your feedback. Let's wait for more feedback. I am sure we will find a good solution in the long run.