Skip to content
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

feature: new strictNot option, enabled by default #493

Open
medfreeman opened this issue Apr 26, 2021 · 2 comments
Open

feature: new strictNot option, enabled by default #493

medfreeman opened this issue Apr 26, 2021 · 2 comments

Comments

@medfreeman
Copy link
Contributor

medfreeman commented Apr 26, 2021

Hi, thanks again for your positive responses!
This is the last update until discussing the issue with multiple index signatures being sometimes generated with patternProperties.

The status of this feature is not totally complete i guess.
You can have a look at test/not_test.ts in my PR #492, but i'm not sure of the consistency of the expected results.

The rationale is the behavior when a not keyword is encountered.
nb: this PR preserves the void output for openAPI responses.

Let me explain my usage of your module:
it's used in the context of an API-first backend.
i use json-schemas to validate collection files (in a format really close to json hyper-schema), with strict requirements (such as union / oneOf with some members having forbidden properties).
but then, after conversion by dtsgenerator, i need not properties to not cancel the positive definitions by producing a void type, but have the definition as if the not keyword was not present, since some combinations are not always directly expressible in typescript.

Now that i'm writing this, i'm thinking perhaps it would be better to generate precise never keys whenever possible, and remove the new option entirely.
Note that this PR introduces the false value in normalized schemas (which makes sense since boolean is an allowed type in json-schema), also used when there's no value such as in the empty openAPI response case.
Imo this part should be kept anyway, since it allows for a more correct not keyword handling.

Let me know what you think about this.

PR #492

@horiuchi
Copy link
Owner

Thank you for your contribution!

It is true that not property is very difficult to handle.
Currently, the schema has been converted to use not only if it is the false schema, so that it eventually becomes a void type (now, never is better).

I looked at test/not_test.ts in the PR, and not: {required: ['id']} is now void type, which is a problem. If this is the case, not is not supported. Rather than the void type, I think it would be better to drop it that any type is unsupported.
Eventually, we should normalize all properties, including allOf, oneOf, not, if, etc., and consider what happens to the value of each property (only A, A & B, A || B, A || ¬ B, etc.). However, we are not able to do that yet.

As for the implementation details, you have added a new type to the return type of the normalizeContent function, but this type is essentially unnecessary. Originally, one of the purposes of this function was to remove the boolean type schema because it makes subsequent processing cumbersome if it is included in the function. Therefore, in the normalizeSchemaContent function, the argument type JsonSchema contains boolean type, but the return type JsonSchemaObject removes it. If you need boolean in the subsequent processing, you can remove it here to achieve your purpose.

Thank you for raising the issue.
It seems to me that the entire conversion process needs to be reviewed in order to respond correctly.

@medfreeman
Copy link
Contributor Author

Ok, i'll try to do an outline next week.
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants