-
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
Document @kbn/config-schema. #50307
Document @kbn/config-schema. #50307
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
@restrry @kobelb it's my first attempt to document this library, any suggestions and criticism would be highly appreciated! |
💔 Build FailedNot relevant to this PR since it touches only |
ACK: reviewing now! |
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 is incredibly helpful! I really like the format. All I have is nitpicks because english is a horrible language. Is English better or worse than JavaScript?
# `@kbn/config-schema` — The Kibana config validation library | ||
|
||
`@kbn/config-schema` is a TypeScript library inspired by the Joi and designed to allow run-time validation of the | ||
Kibana configuration entries providing developers with a fully typed model of the validated data. |
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.
allow run-time validation of the Kibana configuration entries
This is definitely how this started, but it's used in more situations now. For example, at least currently, it's used for HTTP route validation. Perhaps we should keep the current docs until we figure out the scope of where we want this to be used long-term in the NP and rename the package and update the docs at this point?
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.
Yeah, its main official focus is still configuration and if decide to recommend using it for anything else we can update this doc then.
Co-Authored-By: Brandon Kobel <[email protected]>
… to TS docs from literal schema section.
… first schema violation and unify notes sections.
@elasticmachine merge upstream |
Hey @elastic/kibana-platform can someone please take a look at this PR? |
This comment has been minimized.
This comment has been minimized.
💚 Build Succeeded |
|
||
Validates input data as an object with a predefined set of properties. | ||
|
||
__Output type:__ `{ [K in keyof TProps]: TypeOf<TProps[K]> } as TObject` |
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 are no references to TProps
in the interface. BTW, if we had those types as comments in TSDoc it would solve the problem.
As a by-product:
- we can be sure that comments are always in sync with code.
- have suggestions in IDE.
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 are no references to TProps in the interface.
True, I struggled to find a better way to describe properties type here. Do you have any suggestions?
BTW, if we had those types as comments in TSDoc it would solve the problem
Yeah, but it sounds like a bigger separate effort, I'd leave it for a follow-up.
``` | ||
|
||
__Notes:__ | ||
* Conditional schemas may be hard to read and understand and hence should be used only sparingly. |
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 feel the same every time I see rxjs.iif
Would it be useful to follow their naming convention and use trueResult/falseResult
instead of TConditional1/TConditional2
?
Thanks everyone for review! PR includes only md file change so won't wait for CI run this time. |
7.x/7.6.0: 52d15e4 |
The attempt to document @kbn/config-schema based on my past knowledge and memory.
cc @jinmu03
Fixes: #39727
[skip-ci]