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

Events #17402

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Events #17402

wants to merge 15 commits into from

Conversation

aaazzam
Copy link
Collaborator

@aaazzam aaazzam commented Mar 6, 2025

This PR does a few things:

  • uses orval to generate zod models to complement our autogenerate types. This lets us skip defining searchSchemas on every route.
  • uses openapi-ts-query which extends our getQueryService to a full $api object which auto generates queryOptions. I suspect we can eschew a lot of our custom queryKey factories if we like this pattern.
  • turns on codesplitting by route
  • does a POC on the /events page to feel the new DX.

this PR now also includes a backend change:


export const Route = createFileRoute("/events")({
component: RouteComponent,
validateSearch: zodValidator(readEventsEventsFilterPostBody),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What else might we use the generated zod types for besides search params? Because I'm not sure how often the search params actually 1:1 line up with the api requests.

@devinvillarosa love to hear your thoughts on this too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use the generated zod types for our forms as well.

But I think one of root issues is that our typings from openAPI is not 100% accurate.

I've had to add some additional logic to work with the generated typings (such as adding fields that are marked as required, but they should be optional)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think managing the imperfections will get more difficult with these generated zod types? Or could we extend them with the changes we need?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they should be optional

yeah for me I feel like working with the generated client from the server forces the server to be good.

IMO most code we write on the front to account for server typing mistakes is a smell

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaazzam I don't disagree, but I wonder if it will be something we just have to do sometimes because of backwards compatibility and cross compatibility requirements. Ideally every type issue we find we could just fix api side (been doing that already) 🤞

@pleek91
Copy link
Contributor

pleek91 commented Mar 6, 2025

If this can eliminate the manual query keys that would be awesome 👨‍🍳

@aaazzam aaazzam requested review from cicdw and zzstoatzz as code owners March 6, 2025 21:28
Copy link

codspeed-hq bot commented Mar 6, 2025

CodSpeed Performance Report

Merging #17402 will not alter performance

Comparing events (2a7c6fa) with main (70b052c)

Summary

✅ 2 untouched benchmarks

@aaazzam
Copy link
Collaborator Author

aaazzam commented Mar 7, 2025

heads up @pleek91 i'm breaking this up b/c it's a little big IMO

first one is #17413

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui-replatform Related to the React UI rewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants