-
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
Use HTTP request schemas to create types, use those types in the client #59340
Conversation
@@ -5,6 +5,11 @@ | |||
*/ | |||
|
|||
import { SearchResponse } from 'elasticsearch'; | |||
import { schema, TypeOf } from '@kbn/config-schema'; |
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.
Not about your code changes but should we break up the types.ts
file into smaller files specific to an API like:
A file for alerts
, resolver
, metadata
and then have an index.ts
file that puts them altogether so this types.ts
file doesn't get too large?
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.
agree. i figure if this PR is well accepted we can do that before merging
Also I think there is an outstanding issue with using |
I believe @jfsiii ran into this too and as a temporary measure had to define TS types (the type XType = TypeOf) to use in the frontend. |
Yes I don't believe this is going to work correctly until either #57726 is fixed or we upgrade to TypeScript 3.8 (#57774) and can start using type-only imports. I believe the underlying problem is that when we import |
@jonathan-buttner @joshdover thanks for the info. I wasn't aware of those issues. I'm also not experiencing any problems with this PR. Thinking through this: maybe this PR works because the client only imports types and any other @kbn/config-schema stuff is shaken off? Also, to clarify, this code does not use @kbn/config-schema in the client, just the types derived from it. |
@@ -205,7 +205,12 @@ export interface HttpRequestInit { | |||
|
|||
/** @public */ | |||
export interface HttpFetchQuery { | |||
[key: string]: string | number | boolean | undefined; | |||
[key: string]: |
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.
The underlying code supports this already. This change loosens up the type
Ah ok, I missed that, that might work then since it's just doing the types and not the |
Hmm I'm skeptical, because from my understanding this is not how things currently work. If you're importing a module, even just to get the types, that module's dependencies must also be imported & bundled. This is why the type-only imports and exports feature was added to TypeScript. If this is working, I would like to understand why. |
@@ -610,7 +610,7 @@ export interface HttpFetchOptionsWithPath extends HttpFetchOptions { | |||
// @public (undocumented) | |||
export interface HttpFetchQuery { | |||
// (undocumented) | |||
[key: string]: string | number | boolean | undefined; | |||
[key: string]: string | number | boolean | undefined | Array<string | number | boolean | 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.
generated docs
/** | ||
* Used to validate GET requests against the index of the alerting APIs. | ||
*/ | ||
export const alertingIndexGetQuerySchema = schema.object( |
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 we go w/ this design, the schemas would be centralized in common somewhere. I think
), | ||
sort: schema.maybe(schema.string()), | ||
order: schema.maybe( | ||
schema.string({ |
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.
Unrelated to this PR, but I think we could use a oneOf type thing here?
type KbnConfigSchemaInputObjectTypeOf< | ||
T extends kbnConfigSchemaTypes.ObjectType | ||
> = T extends kbnConfigSchemaTypes.ObjectType<infer P> | ||
? { |
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.
This maps the properties of a schema object. It does it twice, once for values that accept 'undefined', and once for keys that do not. If a value accepts undefined, they key is marked optional (?)
TODO add above in comment in code
|
||
export const alertMiddlewareFactory: MiddlewareFactory<AlertListState> = coreStart => { | ||
return api => next => async (action: AppAction) => { | ||
next(action); | ||
const state = api.getState(); | ||
if (action.type === 'userChangedUrl' && isOnAlertPage(state)) { | ||
const response: AlertResultList = await coreStart.http.get(`/api/endpoint/alerts`, { | ||
query: apiQueryParams(state), | ||
query: cloneHttpFetchQuery(apiQueryParams(state)), |
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.
The http.get
function won't accept our Immutable
type. I'm not sure if this is the best approach long term.
Basically, we require that the return value of apiQueryParams
not be modified, and .get
isn't promising that it won't modify them. Therefore we clone it.
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.
Seems okay to me.
|
||
import { HttpFetchQuery } from '../../../../../src/core/public'; | ||
|
||
export function cloneHttpFetchQuery(query: Immutable<HttpFetchQuery>): HttpFetchQuery { |
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.
simply clone a query object.
@joshdover I don't really know. If i had to guess I'd just say 'tree-shaking?'. But I can dig into it more. |
Tree-shaking seems to be the answer. I ran webpack with the stats.json output to use with webpack-bundle-analyzer. What I found is that if you import only types from a module, then the module does not end up in the bundle. As soon as a value is imported, the entire module and it's dependencies will also be imported into the bundle, causing a break. So it appears this will work as long as the module does not contain any values that also need to be imported. Either way, CI / optimizer will let you know when this happens if one of the dependencies needed is not supported in the browser (like the |
@oatkiller @joshdover #59340 (comment) matches my experience as well. Ingest Manager was able to use the We put the |
Thanks for the info. I'm thinking of going w/ the approach I have here in light of the fact that config-schema will likely eventually work in the client. Thanks again |
> = T extends kbnConfigSchemaTypes.ObjectType | ||
? KbnConfigSchemaInputObjectTypeOf< | ||
T | ||
> /** The schema.number() schema accepts strings, so accept them here to. There's no good way to make this happen ONLY when schema.number is called? i dont think it matters? */ |
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.
improve comment verbiage
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.
Typo: to
should be too
.
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.
I thiiiiink this won't matter if strings are accepted everywhere. That's probably due to the fact that URL query params (which are always strings) are accepted?
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.
changed
asc = 'asc', | ||
desc = 'desc', | ||
} | ||
export type Direction = 'asc' | 'desc'; |
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.
@oatkiller Is type
better than enum
? TypeScript n00b here.
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.
Other plugins have used enum for this... for example, siem. So just curious what the advantages/disadvantages are.
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.
I didn't have a strong reason for changing this, was just getting around some quick type errors. Let me try and put it back.
The tl;dr is: enum's end up in the runtime. I think they're good for giving names to magic numbers that you use for data interchange, e.g. binary header sentinels, status codes, etc.
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.
so i tried to go back and make everything the enum, but it turns out we have a whole lot of places in the app writing 'asc' and 'desc' as literals. I don't honestly know what the best practice is, but for the sake of keeping the (runtime) enum out of the validation that will be used on FE and BE, I'm going to keep this as a type for now. I'm aware that this may not be right, I just don't have quite enough experience w/ this yet.
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.
Nah, that's fine. I imagine that could be difficult to work into the schema validation. Let's come back to it later.
/** | ||
* Like TypeOf, but provides a type for creating the value that will match. | ||
* schema.number accepts a string, so this allows strings for number | ||
* schema.maybe creates an optional type, if such a type is a prop on an schema.object, |
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.
Is it technically accurate to say "optional type" here? I see that it's clarified on the line below, but this comment seems a little ambiguous.
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.
reworded. Hopefully more clear?
/**
* Takes a @kbn/config-schema 'schema' type and returns a type that represents valid inputs.
* Similar to `TypeOf`, but allows strings as input for `schema.number()` (which is inline
* with the behavior of the validator.) Also, for `schema.object`, when a value is a `schema.maybe`
* the key will be marked optional (via `?`) so that you can omit keys for optional values.
*
* Use this when creating a value that will be passed to the schema.
* e.g.
* ```ts
* const input: KbnConfigSchemaInputTypeOf<typeof schema> = value
* schema.validate(input) // should be valid
* ```
*/
/** | ||
* Query params to pass to the alert API when fetching new data. | ||
*/ | ||
export type AlertingIndexGetQueryInput = KbnConfigSchemaInputTypeOf< |
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.
👏 👏 👏
@@ -39,11 +34,11 @@ export class AlertDetailsPagination extends Pagination< | |||
): Promise<SearchResponse<AlertEvent>> { | |||
const reqData: AlertSearchQuery = { | |||
pageSize: 1, | |||
sort: EndpointAppConstants.ALERT_LIST_DEFAULT_SORT, |
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.
Why dis?
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.
can restore it. the value in the constants class was not typed right so i just used the literal
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.
restored it
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.
restored it
This reverts commit 5d40ca18337cf8deb63a0291150780ec094db016.
Pinging @elastic/endpoint-app-team (Feature:Endpoint) |
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.
Platform changes LGTM
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Saved Object API Integration Tests -- security_only.x-pack/test/saved_object_api_integration/security_only/apis/bulk_update·ts.saved objects security only enabled bulkUpdate dual-privileges user "before all" hook for "should return 403 for hiddentype doc"Standard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / X-Pack Saved Object API Integration Tests -- security_only.x-pack/test/saved_object_api_integration/security_only/apis/bulk_update·ts.saved objects security only enabled bulkUpdate dual-privileges user "after all" hook for "should return 403 for hiddentype doc"Standard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / X-Pack Saved Object API Integration Tests -- security_only.x-pack/test/saved_object_api_integration/security_only/apis/bulk_update·ts.saved objects security only enabled bulkUpdate dual-privileges user "before all" hook for "should return 403 for hiddentype doc"Standard Out
Stack Trace
History
To update your PR or re-run it, just comment with: |
* master: (22 commits) Generate docs from data plugin (elastic#56955) Fixes elastic#59513 by hiding one of the symmetric edges rather than omiting it (elastic#59514) Harden creation of child processes (elastic#55697) [Alerting] replace watcher http APIs used by index threshold Alerting (elastic#59475) [Maps][docs] add more details to Quantitative data driven styling docs (elastic#59553) chore: 🤖 hide Drilldowns in master (elastic#59698) [Discover] Migrate AppState/GlobalState to new app state helpers (elastic#57175) Use HTTP request schemas to create types, use those types in the client (elastic#59340) [Maps] Support categorical styling for numbers and dates (elastic#57908) [ML] Functional API tests - bucket span estimation with custom search.max_buckets (elastic#59665) Fix slm_ui setting by changing camel case back to snake case. (elastic#59663) removes beta tag (elastic#59618) [DOCS] Removed spatial references (elastic#59595) fix outdated docs (elastic#58729) [ML] Fixes bucket span estimators loading of max_buckets setting (elastic#59639) [ML] Refactoring anomaly detector job types (elastic#59556) [Upgrade Assistant] Better handling of closed indices (elastic#58890) additional visualizations plugin cleanup before moving to NP (elastic#59318) In scripted fields, unable to switch the `Type` - getting a console error which says - Class constructor DecoratedFieldFormat cannot be invoked without 'new' (elastic#59285) [Visualize] Remove global state in visualize (elastic#58352) ...
…s/kibana into alerting/fix-flaky-instance-test * 'alerting/fix-flaky-instance-test' of github.com:gmmorris/kibana: (176 commits) Generate docs from data plugin (elastic#56955) Fixes elastic#59513 by hiding one of the symmetric edges rather than omiting it (elastic#59514) Harden creation of child processes (elastic#55697) [Alerting] replace watcher http APIs used by index threshold Alerting (elastic#59475) [Maps][docs] add more details to Quantitative data driven styling docs (elastic#59553) chore: 🤖 hide Drilldowns in master (elastic#59698) [Discover] Migrate AppState/GlobalState to new app state helpers (elastic#57175) Use HTTP request schemas to create types, use those types in the client (elastic#59340) [Maps] Support categorical styling for numbers and dates (elastic#57908) [ML] Functional API tests - bucket span estimation with custom search.max_buckets (elastic#59665) Fix slm_ui setting by changing camel case back to snake case. (elastic#59663) removes beta tag (elastic#59618) [DOCS] Removed spatial references (elastic#59595) fix outdated docs (elastic#58729) [ML] Fixes bucket span estimators loading of max_buckets setting (elastic#59639) [ML] Refactoring anomaly detector job types (elastic#59556) [Upgrade Assistant] Better handling of closed indices (elastic#58890) additional visualizations plugin cleanup before moving to NP (elastic#59318) In scripted fields, unable to switch the `Type` - getting a console error which says - Class constructor DecoratedFieldFormat cannot be invoked without 'new' (elastic#59285) [Visualize] Remove global state in visualize (elastic#58352) ...
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
Use a type derived from @kbn/config-schema on the client when creating requests.
Backstory
When creating an HTTP route, we use
@kbn/config-schema
to create 'schema' objects. These schemas are used to validate and coerce the inputs to the HTTP handler (e.g. HTTP query strings, POST bodies.) The client will make HTTP requests to these handlers. If the HTTP requests made by the client fail the validation, an HTTP error will be triggered. In that case an error will likely be shown to the user.In the HTTP handler code the validated (and coerced) inputs are cast to a type based on the schema. This is called the
TypeOf<schema>
type. This allows the HTTP handlers to have type safety.If the schema changes, the HTTP handler will get a new type, and the developer will be informed (by typecheck) that the HTTP handler code must change.
Ideas
This PR is a POC. It defines a pattern for extending the type safety to the client. The idea is this:
TypeOf<schema>
type, as beforeKbnConfigSchemaInputTypeOf<schema>
type. This type represents the valid inputs to the request.With this pattern, if the schema changes, the developer will also be notified that the code creating the request needs to be changed.
Gotchas and stuff
The inputs are more permissive than the validated types due to coercion. For example, the schema
schema.number()
accepts strings, and will automatically convert these to numbers.Another reason: the
TypeOf
type has limited handling of theschema.maybe
schema when used withschema.object
Checklist
Delete any items that are not applicable to this PR.
(https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
For maintainers