-
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
Allow to extend ObjectType schemas #67357
Allow to extend ObjectType schemas #67357
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
@pgayvallet This is great, thanks for working on this! Do you mind including an example of how to use this properly? |
@chrisronline is the jsdoc on |
@pgayvallet Ah, I didn't notice that in the PR. That's exactly what I wanted! |
I don't really love this pattern because it exposes some implementation details of config-schema outside of the package. Is there any reason this API wouldn't work? const origin = schema.object({
initial: schema.string(),
});
const extended = origin.extend({
added: schema.number(),
}); In the future this |
As @restrry said in #59083 (comment), the limitation of the const origin = schema.object({
initial: schema.string(),
toRemove: schema.string()
});
const extended = origin.extend({
toRemove: undefined, // error, undefined does not extends `Type<any>`, and can't easily change typings / implementation to allow it I think?
}); |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/dashboard/_async_dashboard·ts.dashboard sample data dashboard "before all" hook for "should launch sample flights data set dashboard"Standard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/dashboard/_async_dashboard·ts.dashboard sample data dashboard "after all" hook for "toggle from Discover to Dashboard attempt 2"Standard Out
Stack Trace
History
To update your PR or re-run it, just comment with: |
Missed that comment, however I think this would still be possible from a TypeScript perspective. interface A {
initial: string;
toRemove: string;
}
interface B {
toRemove: undefined;
}
type C = Omit<A, keyof B> & B;
type D = C['toRemove'] // undefined Interface for extend could look like: public extend<NP extends Props>(newProps: NP): ObjectType<Omit<P, NP> & NP>; Could also use |
public extend<NP extends Props>(newProps: NP): ObjectType<Omit<P, NP> & NP>; I'm not sure this is as simple:
schema.object({ doesntMakeSenseImho: undefined })
I think we could add a public extend<NP extends NullableProps>(newProps: NP): ObjectType<Omit<P, NP> & NP>; with type NullableProps = Record<string, Type<any> | undefined | null>; but we would still need to adapt the (already complicated) I can try the approach if we think this is better. It does avoid leaking some 'internals' of the object schema, however I'm not sure it's really easier to use, as omitting keys from a schema seems like more natural than setting them to |
created #68067 as an alternative. See #59083 (comment) for the discussion |
Question: do we have a real use case in mind that requires extending a schema + removing some keys? |
Not on my side. @chrisronline ? |
We need to take the existing Elasticsearch config schema and:
|
Well, anyway, it's would now be possible with both implementations 😄 @joshdover tell me if I should close this PR in favor of #68067. |
Closing in favor of #68067 |
Summary
Fix #59083
Add a
getRaw
method onObjectType
returning the raw schema properties used to create the type. This allows to create new schemas with altered properties.Checklist