Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Another notation for FormDataInfo #5

Open
sean-nicholas opened this issue Oct 31, 2023 · 11 comments
Open

Another notation for FormDataInfo #5

sean-nicholas opened this issue Oct 31, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@sean-nicholas
Copy link

What do you thing about a new notation for FormDataInfo? Something that looks more like the object that will get returned:

const formValues = decode(formData, {
  title: 'string',
  price: 'number',
  created: 'date',
  active: 'boolean',
  tags: ['string'],
  images: [{
      title: 'string',
      created: 'date',
      file: 'file',
    }]
  ,
});

Benefits:

  • imho the DX is better: I know how my object should look like in the end. Its way easier to write down the object compared to paths where I want a transformation to happen.
  • It pretty much looks like a valibot / zod schema. So switching between "pure" FormDataInfo and a Schema is super easy
@sean-nicholas sean-nicholas changed the title FormDataInfo Another way to write FormDataInfo Oct 31, 2023
@sean-nicholas sean-nicholas changed the title Another way to write FormDataInfo Another notation for FormDataInfo Oct 31, 2023
@fabian-hiller
Copy link
Owner

The idea is great. However, there are some points that speak against it. I appreciate feedback on this.

  • decode only needs information about certain data types. A whole schema is not needed.
  • If we remove the unneeded information, like title: 'string', the API could be a bit confusing, as it is not obvious which datatypes need to be added.
  • decode needs the data exactly as it is currently passed. This means we would have to parse that object first and convert it to the current format. This takes additional code and CPU time.

@fabian-hiller fabian-hiller self-assigned this Nov 1, 2023
@fabian-hiller fabian-hiller added the enhancement New feature or request label Nov 1, 2023
@sean-nicholas
Copy link
Author

decode only needs information about certain data types. A whole schema is not needed.

I guess this a question of personal taste. I would like to have the unneeded information in FormDataInfo to see the shape of the output. But I guess other people would find that too much of a hassle.

If we remove the unneeded information, like title: 'string', the API could be a bit confusing, as it is not obvious which datatypes need to be added.

Yeah, I wouldn't do this, too. Either the whole object or keep staying with the current FormDataInfo. Everything else is confusing.

decode needs the data exactly as it is currently passed. This means we would have to parse that object first and convert it to the current format. This takes additional code and CPU time.

Two options here:

  • Refactor: decode could iterate over the "new" FormDataInfo object and only put formData in the output that is specified in FormDataInfo (possible security feature, see "More benefits").
  • Just ignore the additional CPU time. Parsing formData does not happen so many times and I guess the transformation is not really expensive.
    About the additional code: Won't you write something similar for valibot & zod? How do you want to handle the additional code for that? Adapters? Then this "new" FormDataInfo format could be encapsulated in an adapter, too. This would fix downward compatibility, too. But reduces DX because you would need to import two things from the lib.

More benefits:

  • You might be able to infer the Output based on the FormDataInfo.
  • Dropping non expected fields could be a security feature. Only fields that are specified in FormDataInfo will be added to the output. So no one could add unexpected fields into the form. But maybe valibot / zod would be the better alternative in this case.

@fabian-hiller
Copy link
Owner

Thank you for your feedback. How should decode behave if title: 'string' is defined but FormData does not contain a title field?

@sean-nicholas
Copy link
Author

I thought for a long time about it. I think there are three possible solutions:

  1. Ignore it. So title is undefined
  2. Throw an error
  3. Let the consumer (developer) decide

I think there are valid cases for both 1. & 2. For example:

-> 1.: The field is really optional and can be added or removed client side. You are okay if it is undefined.
-> 2.: Some error happend on the client and the field should not be missing. So the consumer wants decode to throw. (Possibly it would be okay to still return undefined but the the consumer has to handle the parsed output with zod / valibot / themself.)

Third option is tricky: If you start to let the consumer decide on a per field basis (like title: 'string?') you start reimplementing zod / valibot. And question like "how to mark an array as optional?" will arise.

But you could let the consumer decide on a global basis. Ultimately, I think the following api could be nice:

  • decode --> Throws if title is undefined or the date can't be parsed
  • safeDecode --> returns { hasParsingErrors: boolean, parsingErrors: (some array that shows paths and errors), data: (the object with undefined values where an error occued) }
    data would always be defined regardless if hasParsingErrors is true or false. parsingErrors would be only defined if hasParsingErrors is true.

Maybe safeDecode is not the best wording because one might expect a return value that is like zods safeParse.

@fabian-hiller
Copy link
Owner

My fear is that the new notation will resemble a schema. Instead of inventing a new schema API, I would prefer to provide adapters for Valibot, Zod and other libraries. On the one hand, an own schema increases the complexity of this library, and on the other hand I have to maintain a second schema besides Valibot. Would you use your schema notation at all if there was an integration to Valibot?

@sean-nicholas
Copy link
Author

Yeah. I would totally use valibot / zod for that. I think that is the best solution. As you said the risk to create a second schema is way to high.

@fabian-hiller
Copy link
Owner

Good to know. I will try to implement a first draft for Valibot in the next weeks. I'm still not sure how to structure this. I think that decode-formdata should remain dependency free. One solution would be to publish a schema specific decode functions for Valibot under @decode-formdata/valibot. What do you think about that? Do you have alternative ideas?

@patrickpissurno
Copy link

If you mean a version of the decode function that can take in a Valibot schema for its second parameter, then that'd be good. That way we could have a single source of truth for the schema.

Internally, it could be implemented as a function that traverses Valibot schemas converting them into FormDataInfo-compliant objects. Honestly, it'd be even more useful if this function was exposed directly (like an adapter), so that it could be called once and have the results stored in a variable for subsequent decode calls.

I don't see much value in it calling decode directly, because then you'd incur the perf. costs for every call, as opposed to only once.

What do you think?

@fabian-hiller
Copy link
Owner

Good idea! I'll take that into consideration. Do you have any idea what you would call the adapters?

@patrickpissurno
Copy link

patrickpissurno commented Nov 28, 2023

Maybe fromSchema() would do it. It could then be used somewhat like this:

import { decode } from 'decode-formdata';
import { fromSchema } from 'decode-formdata/valibot';

const LoginSchema = /* ... */;

// for folks who aren't as concerned with the added perf. cost
decode(formData, fromSchema(LoginSchema)); // elsewhere inside a function

// alternatively
const decoder = fromSchema(LoginSchema);

decode(formData, decoder); // elsewhere inside a function

This is the best I could think of, but I'm not great at naming things haha. What I do like about it is how composable it sounds to me.

By the way, really appreciate the work you've been doing with Valibot and these other libs. Top-notch stuff indeed.

@fabian-hiller
Copy link
Owner

Thank you for your kind words and feedback! I will probably look at the implementation in late December or January.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants