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

[kbn/server-route-repository] Make params optional for Zod schema with only optional keys #191709

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

miltonhultgren
Copy link
Contributor

@miltonhultgren miltonhultgren commented Aug 29, 2024

io-ts and zod have differences in how they usually define optional keys.

In io-ts, we usually define a union between a type and a partial, and if all keys are optional we only provide the partial type.
While in zod if all keys are optional, it's more common to simply put each key as optional while the wrapping object is not.

The way the type inference works in the route repository leads to this two cases having different results.
For io-ts, when it sees a partial, it marks the query/path/body and thus params object as fully optional, while for zod since query/path/body are not optional but all their keys are, the chain is still marked as required.

This PR aims to align these behaviours by checking if the zod schema only specifies optional field, and if it does it marks the query/path/body level as optional so that params also can become optional.

This way, when calling the API via the client, if all params are optional, the second options argument can be omitted.
Similarly, the types in the handler are marked correctly as possibly undefined to prevent bugs.

The PR also adds a little isHttpFetchError helper with slightly better type support than what the Core version has.

@miltonhultgren miltonhultgren added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Aug 29, 2024
@miltonhultgren miltonhultgren requested a review from a team as a code owner August 29, 2024 09:38
@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/server-route-repository-client 3 9 +6
Unknown metric groups

API count

id before after diff
@kbn/server-route-repository-client 3 9 +6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

lgtm

@miltonhultgren miltonhultgren merged commit 6154420 into elastic:main Aug 29, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants