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

json-schema: v6 and v7 changed to better comply with the draft definitions. #29026

Merged
merged 4 commits into from Sep 20, 2018

Conversation

afinch7
Copy link
Contributor

@afinch7 afinch7 commented Sep 19, 2018

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

"type": ["object", "boolean"]

@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Sep 19, 2018
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Sep 19, 2018
@typescript-bot
Copy link
Contributor

typescript-bot commented Sep 19, 2018

@afinch7 Thank you for submitting this PR!

🔔 @bcherny @cyrilletuzi @LucianBuzzo @rolandjitsu - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@cyrilletuzi
Copy link
Contributor

@afinch7 What is the motivation of this PR? I don't see what it adds except refactoring, and I'm not confident in validating the changes, change from interface to type could have side effects.

@rolandjitsu
Copy link
Contributor

@cyrilletuzi I second that. I cannot see how a schema object can be a boolean, at least not the root schema object.

@afinch7
Copy link
Contributor Author

afinch7 commented Sep 19, 2018

According to the draft 07 and 06 specs any field that can be a schema object can be a boolean as well.

Note this declaration in the 07 and 06 schemas. (see http://json-schema.org/draft-07/schema# and http://json-schema.org/draft-06/schema#)

"type": ["object", "boolean"]

This means that a schema like this

{
  "type": "object",
  "properties": {
    "aProp": false
  }
}

while mostly pointless is still valid, and more importantly

{
  "type": "object",
  "additionalProperties": false,
  "properties": {
    "aProp": false
  }
}

is valid.

I could have changed the definition of additionalProperties to be "boolean | JSONSchema6/7", but this doesn't correctly reflect the schema definition.

TL;DR;
The "additionalProperties" property was not functioning correctly before. Allowing for Schemas to be bools as well should better reflect the underlying spec, and fix this issue.

@afinch7
Copy link
Contributor Author

afinch7 commented Sep 19, 2018

@cyrilletuzi As for the interface to type change. I do see the potential for problems.
I.E.

class SomeClass implements JSONSchema7 

This will no longer compile.

I see there being two options to resolve this. Make this a breaking change or refactor this change to something like

type JSONSchemaSomething7 = JSONSchema7  | boolean;

interface JSONSchema7 {
  properties: JSONSchemaSomething7;
}

There may be a better way to refactor that not sure. What do you think?

@rolandjitsu
Copy link
Contributor

@afinch7 I'm aware of that, but making the root schema a boolean makes no sense, especially when you're extending the interface or implementing it.

Your second suggestion would work though.

…hema6/7 and added test to avoid breaking this in the future.
@afinch7
Copy link
Contributor Author

afinch7 commented Sep 19, 2018

Alright I created a test for the class implementation cases and
renamed my new type to JSONSchema6Definition and JSONSchema7Definition.
afinch7/DefinitelyTyped@0d7ab87

Refactored my changes to avoid breaking class implementations.
Copy link
Contributor

@rolandjitsu rolandjitsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Check and Merge in Pull Request Status Board Sep 20, 2018
@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express and removed Awaiting reviewer feedback labels Sep 20, 2018
@typescript-bot
Copy link
Contributor

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@jessetrinity jessetrinity merged commit cb75e9c into DefinitelyTyped:master Sep 20, 2018
@typescript-bot typescript-bot removed this from Check and Merge in Pull Request Status Board Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Owner Approved A listed owner of this package signed off on the pull request. Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants