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

feat: use zod for transformation and validation #1777

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Feb 22, 2024

closes #1489

PoC pull request to display, and discuss, the implications of using zod for class transformation and validation.

Right now, only did the action menu package, it is quite the easy transition and I really do prefer the API over class-transformer and class-validator. The decorators are quite nice, but in general it is quite limited and annoying to work with.

I compared it to arktype, but mainly based on the download count (7.1m vs 22.6k) weekly I chose for zod to make an initial PoC with. I think both have some great advantages, but zod seems more mature and well-established.

We will likely have to do the entire conversion in one PR as the core uses class-transformer for all the records which may be defined in a package.

Another thing is that zod can provide quite expanded errors and this is something we can rely on very heavily to inform the user.

One thing that I noticed as well is that the zod schemas work really well when extending eachother so we can likely reuse a lot of schemas internally. Maybe we can expose some schemas under a validation object publically so other packages may use it.

Curious to hear if there are any opinions on this, whether we should move forward and possibly if someone would like to help with some of the classes. I'd be happy to setup a base which we can reuse, but it will be quite quite quite some lines.

@berendsliedrecht berendsliedrecht force-pushed the zod branch 3 times, most recently from 9b0b629 to 2f6d627 Compare February 23, 2024 09:58
@TimoGlastra
Copy link
Contributor

Nice stuff @berendsliedrecht!! I'd really like this getting in. It should simplify quite some stuff, but will have a very big impact on existing users of the framework I think... Maybe we can aim to get it in for 0.6

A few questions:

  • I see you also use zod for the options in a message class. How are you intending to do message JSON to class instance parsing? I see that's not part of this PR. Should the agent message class have a fromJson method that each message can implement to parse the JSON into the respective message class? I think the constructor should be different from what is used to parse a message from json (IMO), as there's difference in trying to parse an incoming message and constructing one on our side
  • Should we have a base fromJson implementation in the base class that extracts all decorators? Should we also add something like additionalFields to the base agent message where we can put all unknown claims? Currently if you parse a message from json to a class instance, back to json it will keep the unknown properties (thinkg e.g. a tracing decorator, custom decorators, etc..) We should keep these and not 'lose' these when going to a class instance and back
  • Do we want to keep using class instances for messages? Or is validated JSON objects for messages also enough? This would require a more functional approach to work with message classes?

As you say, it'd be good to add some good primitives in core (and maybe even didcomm package) to make the transition as smooth as possible and consistent.

@berendsliedrecht
Copy link
Contributor Author

Maybe we can aim to get it in for 0.6

Yes, that would be the goal!

  • I see you also use zod for the options in a message class. How are you intending to do message JSON to class instance parsing? I see that's not part of this PR. Should the agent message class have a fromJson method that each message can implement to parse the JSON into the respective message class? I think the constructor should be different from what is used to parse a message from json (IMO), as there's difference in trying to parse an incoming message and constructing one on our side

Yeah that is a good question. Maybe we should look at a pattern where the constructor is not used and we have a fromMessage / fromJson and fromMessageUnsafe / fromJsonUnsafe (or unvalidated). Each message class could have a property like parsingSchema which contains the zod schema and fromMessage / fromJson would only have to be implemented once on the BaseMessage class. With this setup, I'd avoid using a constructor as it will be unclear whether it will validate or not. Or we should have something like public constructor(ops: Options, additionalOps?: {shouldValidate: boolean}) which is not really a great API imo.

  • Should we have a base fromJson implementation in the base class that extracts all decorators? Should we also add something like additionalFields to the base agent message where we can put all unknown claims? Currently if you parse a message from json to a class instance, back to json it will keep the unknown properties (thinkg e.g. a tracing decorator, custom decorators, etc..) We should keep these and not 'lose' these when going to a class instance and back

Ah that is something I did not think of, yet. If the current and intended behavior is to keep the unknown fields of the JSON then that should stay for sure. IIRC zod has something for this, but I would have to dive into it again and find the exact API.

  • Do we want to keep using class instances for messages? Or is validated JSON objects for messages also enough? This would require a more functional approach to work with message classes?

Yes, I think classes are the way to go here. This way we also still have the properties of like the didcomm message url, etc. If we would just use objects it would still be possible, but I think using it will become more annoying. If we want to move away from class instances for things like this, I think we should first go through a larger part of the code base as it would feel a bit inconsistent if we only do this for the messages. Also, creating custom modules, etc. is easier if we can just extend objects and use decorators like @Injectable.

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

Successfully merging this pull request may close these issues.

Migrating away from class-transformer and class-validator
2 participants