-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[APM] Extract server type utils to package #96349
[APM] Extract server type utils to package #96349
Conversation
Pinging @elastic/uptime (Team:uptime) |
Pinging @elastic/apm-ui (Team:apm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only skimmed this but overall looks great.
Big 👍 to extracting this and making it available to other teams
// It does not allow params for routes without a params codec | ||
client({ | ||
endpoint: 'endpoint_without_params', | ||
// @ts-expect-error Object literal may only specify known properties, and 'params' does not exist in type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of @ts-expect-error
👍
// @ts-expect-error Property 'noParamsForMe' is missing in type | ||
const invalidType: { | ||
noParamsForMe: boolean; | ||
} = res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have access to expect
you can double this as both a runtime and compile assertion. And I find the intention a little more clear this way
// @ts-expect-error Property 'noParamsForMe' is missing in type | |
const invalidType: { | |
noParamsForMe: boolean; | |
} = res; | |
// @ts-expect-error Property 'noParamsForMe' is missing in return type | |
expect(res.noParamsForMe).toBe(undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect might not be the appropriate tool, but let me see if I can write some other utility function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An assert generic is pretty cool 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM !!
Can we please add a simple readme file with few examples?
I am very much interested in using these in uptime app.
But i am having hard time understanding it from examples that what advantages it would provide.
@shahzad31 I've added a placeholder - I'd like to work with you on integrating it in Uptime and writing the readme together based on that experience. Please ping me next week or after if you have some time for that! |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Extracts the tooling we use to create fully typed server routes to a separate package for use in the Observability plugin (and possibly others).
For examples, see https://github.com/dgieselaar/kibana/blob/17a48e34970387689a7d6e618dd54f7e77564e91/packages/kbn-server-route-repository/src/test_types.ts