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

Add zod routes to RPC #5570

Draft
wants to merge 3 commits into
base: staging
Choose a base branch
from
Draft

Add zod routes to RPC #5570

wants to merge 3 commits into from

Conversation

danield9tqh
Copy link
Member

Summary

Add ability to implement zod routes

Testing Plan

Documentation

Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference
)? If yes, link a
related documentation pull request for the website.

[ ] Yes

Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and label it with breaking-change-rpc or breaking-change-sdk.

[ ] Yes

@danield9tqh danield9tqh requested a review from a team as a code owner October 21, 2024 20:51
@NullSoldier
Copy link
Contributor

Can you explain what the purpose of this is? It's just changing a schema library to a new one without any explanation.

This is a sort of auto-reject PR without further explanation if the goal is just to change libraries for the sake of change.

  1. What does this random other library do that our current one doesn't do?
  2. What issues are developers facing that this solves?

@danield9tqh
Copy link
Member Author

The main downside of yup right now is it doesn't support some of the typescript types that we would ideally want to handle in our requests. Here's one example: https://github.com/iron-fish/ironfish/pull/5569/files#r1811059203 but there are others
I've run into too. I can look through the code for some examples

Also

  • the node app uses zod and we can re-use some schemas there between the two codebases
  • zod works better with typescript in general we can use utils like z.infer<> to create the request type automatically from the schema.

@NullSoldier
Copy link
Contributor

NullSoldier commented Oct 22, 2024

The main downside of yup right now is it doesn't support some of the typescript types that we would ideally want to handle in our requests. Here's one example: https://github.com/iron-fish/ironfish/pull/5569/files#r1811059203 but there are others I've run into too. I can look through the code for some examples

Also

  • the node app uses zod and we can re-use some schemas there between the two codebases
  • zod works better with typescript in general we can use utils like z.infer<> to create the request type automatically from the schema.

I just commented on the linked example, which does not seem valid, but I could be wrong.

Also, yup has infer too but we explicitly don't use it for various reasons around ambiguety. I had issues with it back in the day but we're on 0.29.x and now it's at 1.0 so we could easily just upgrade and test how well infer has improved.

Did you upgrade yup and try the latest infer behaviour before deciding to throw out the baby with the bath water?

@danield9tqh danield9tqh marked this pull request as draft October 28, 2024 17:23
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.

3 participants