chore: implement JSON Schema to NodeType adapter #1401
Merged
Conversation
This file contains 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
|
|
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success712 tests passing in 101 suites. Report generated by 🧪jest coverage report action from ab62c1a |
tatomyr
force-pushed
the
json-schema-to-node-type-adapter
branch
from
c59c675 to
31d2a6e
Compare
tatomyr
force-pushed
the
json-schema-to-node-type-adapter
branch
from
2fe3a1c to
b72833f
Compare
RomanHotsiy
reviewed
Feb 12, 2024
| ? itemsType(node[i], joinPointer(nodeAbsoluteRef, i)) | ||
| : itemsType; | ||
| // we continue resolving unknown types, but stop early on known scalars | ||
| if (itemType === undefined && type !== unknownType && type !== SpecExtension) { |
There was a problem hiding this comment.
I think this if statement maybe be causing slower benchmark but looks like we need this code
There was a problem hiding this comment.
I did not review this file.
| @@ -126,7 +127,7 @@ export async function lintConfig(opts: { | |||
| rules: { spec: 'error' }, | |||
| }); | |||
|
|
|||
| const types = normalizeTypes(ConfigTypes, config); | |||
| const types = normalizeTypes(opts.externalConfigTypes || ConfigTypes, config); | |||
There was a problem hiding this comment.
I think we need to move to universal plugins approach so won't need it.
Let's talk offline.
tatomyr
force-pushed
the
json-schema-to-node-type-adapter
branch
from
872292a to
672bc50
Compare
tatomyr
added
No Changeset Needed
deploy_s3_sandbox
and removed
No Changeset Needed
labels
tatomyr
force-pushed
the
json-schema-to-node-type-adapter
branch
from
a87cc5f to
f748ab6
Compare
IgorKarpiuk
reviewed
Feb 15, 2024
tatomyr
force-pushed
the
json-schema-to-node-type-adapter
branch
from
f748ab6 to
ef029ff
Compare
…des; add unit tests
…config schemas according to the latest monorepo changes
update perf benchmark switch to museum for a moment improve walk and resolve for performance cleanup the code change fn syntax make the adapter errors nicer
tatomyr
force-pushed
the
json-schema-to-node-type-adapter
branch
from
ef029ff to
ab62c1a
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What/Why/How?
Implemented JSON Schema to NodeType adapter.
This streamlines merging external config schemas while allowing deriving TypeScript types from JSON Schema schemas.
The adapter itself is here. It uses AJV to determine the correct
oneOfschema if there's nodiscriminatordefined.The adapter is limited to JSON Schemas that are not booleans or arrays themselves, nor types are arrays.
Also, the schemas should not contain
allOforanyOf(we're not using them anyway).The most crucial limitation is that an array's-- Upd: fixed in 2fe3a1citemscould not containoneOf. I even had to change the adapters schema schema of thedeveloperOnboardingsection. That is becauseNodeTypecannot alter items (like it can do withpropertiesoradditionalProperties).I also fixed a wrong snapshot test, added missing configurable rules subject types, and updated the config schema to the latest version.
Reference
Testing
Performance
Investigated performance losses. It doesn't do that bad on linting. For the current Rebilly API description it doesn't show any significant difference comparing to other versions:
node node_modules/cli-140/bin/cli.js lint api-definitions/openapi/openapi.yamlnode node_modules/cli-150/bin/cli.js lint api-definitions/openapi/openapi.yamlnode node_modules/cli-160/bin/cli.js lint api-definitions/openapi/openapi.yamlnode node_modules/cli-170/bin/cli.js lint api-definitions/openapi/openapi.yamlnode node_modules/cli-180/bin/cli.js lint api-definitions/openapi/openapi.yamlnode node_modules/cli-next/bin/cli.js lint api-definitions/openapi/openapi.yamlOn the other hand, for smaller museum.yaml API description the losses are higher (which most likely means the config itself gets resolved longer):
node node_modules/cli-140/bin/cli.js lint ../resources/museum.yamlnode node_modules/cli-150/bin/cli.js lint ../resources/museum.yamlnode node_modules/cli-160/bin/cli.js lint ../resources/museum.yamlnode node_modules/cli-170/bin/cli.js lint ../resources/museum.yamlnode node_modules/cli-180/bin/cli.js lint ../resources/museum.yamlnode node_modules/cli-next/bin/cli.js lint ../resources/museum.yamlScreenshots (optional)
Check yourself
Security