Add isConfigSchema typeguard and stop using instanceof Type checks in core#63821
Merged
pgayvallet merged 6 commits intoelastic:masterfrom Apr 17, 2020
Merged
Conversation
Contributor
|
Pinging @elastic/kibana-platform (Team:Platform) |
3a26331 to
3c3e479
Compare
pgayvallet
commented
Apr 17, 2020
Comment on lines
+26
to
+34
| router.get( | ||
| { | ||
| path: '/api/core_config_schema/foo', | ||
| validate: { | ||
| query: schema.object({ | ||
| foo: schema.string(), | ||
| bar: schema.number(), | ||
| }), | ||
| }, |
Contributor
Author
There was a problem hiding this comment.
Note: the presence/addition of this plugin is sufficient to test the fix, as all test suites in test/plugin_functional would fail with Expected a valid validation logic declared with '@kbn/config-schema' package or a RouteValidationFunction at key: [query]when the server loads the plugin. I will remove the plugin when we have other test plugins using route validation, probably in #63549 as it got one.
Contributor
Contributor
Author
There was a problem hiding this comment.
Yea, I agree. I was wondering if I should remove it. I initially added it as the issue is not easily reproductible outside of the CI builds, but now that I got a green build, I can remove it if we think this is better. WDYT?
pgayvallet
commented
Apr 17, 2020
| import { Type } from '../types'; | ||
|
|
||
| export function isConfigSchema(obj: any): obj is Type<any> { | ||
| return obj ? obj.__isKbnConfigSchemaType === true : false; |
Contributor
Author
There was a problem hiding this comment.
I started with the ?. syntax, but it looks like the packages are not transpiled the same way src is, and I was getting CI failures with
return obj?.__isKbnConfigSchemaType === true;
^
SyntaxError: Unexpected token .
in karma tests...
mshustov
approved these changes
Apr 17, 2020
Comment on lines
+26
to
+34
| router.get( | ||
| { | ||
| path: '/api/core_config_schema/foo', | ||
| validate: { | ||
| query: schema.object({ | ||
| foo: schema.string(), | ||
| bar: schema.number(), | ||
| }), | ||
| }, |
Contributor
| expect(isConfigSchema(schema.stream())).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false for every primitive types', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('returns false for every primitive types', () => { | |
| it('returns false for every javascript data type', () => { |
Contributor
Author
|
retest |
Contributor
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Detection Engine API Integration Tests.x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses·ts.detection engine api security and spaces enabled find_statuses should return a single rule status when a single rule is loaded from a find status with defaults addedStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
pgayvallet
added a commit
to pgayvallet/kibana
that referenced
this pull request
Apr 17, 2020
…in core (elastic#63821) * add isConfigSchema type guard * replace instanceof checks with isConfigSchema * add dummy test plugin using a route with validation schema * remove `?.` prop access * remove test plugin * fix test description
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix #61652
Add a isConfigSchema type guard to
kbn/config-schemaand replaceinstanceof Typechecks in core with it.Checklist