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

Add checks for strict null checks #1583

Merged
merged 1 commit into from
May 4, 2021

Conversation

erikbrinkman
Copy link
Collaborator

What issue does this pull request resolve?

It addresses some of the issues with #1521 by checking if strict null checks is enabled, and if not, setting all exported types from json-schema to a string literal indicating the problem.

What changes did you make?

  1. created an internal wrapper type that checks if strictNullChecks is enabled. If so, resolves to the normal type, if not, resolves to a string literal type with an error.
  2. renamed all external types to UncheckedExternalType and made internal, then added an external type that wraps it with the type above.
  3. minor removed the default setting and the exposure of _partial from JSONSchemaType. This shouldn't have been used in the past as one could just use PartialSchema and it was arguably marked as private with the underscore name, but it did still used to be exposed.
  4. minor changed _partial to IsPartial to conform with standard capitalization of types, which is now also less relevant as that parameter isn't exported.

Is there anything that requires more attention while reviewing?

  • I added wrappers around everything that was exported, but potentially that's not necessary, or desired? It's not super clear, so this seemed like the safe option.
  • The case could be made that removing _partial from JSONSchemaType is backwards compatibility breaking. If you're worried about that I can add it back, but ultimately think it should be removed
  • I renamed everything Unchecked... that's pretty verbose and maybe not clear, so I'm open to other names.
  • I've tested this in the typescript playground (the current version), but there are no built-in tests for it. I decided that adding a whole new type of tests that needed a custom tsconfig was overkill for this feature, but I'm open to the idea that it should be tested.

@epoberezkin
Copy link
Member

The case could be made that removing _partial from JSONSchemaType is backwards compatibility breaking. If you're worried about that I can add it back, but ultimately think it should be removed

I don't think anybody used it, it was effectively private, so it's cool that it is fully private now.

@epoberezkin epoberezkin merged commit b3cc337 into ajv-validator:master May 4, 2021
@erikbrinkman erikbrinkman deleted the snc branch May 21, 2021 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants