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

Support boolean values for additionalProperties #1

Merged
merged 1 commit into from
Feb 3, 2021
Merged

Support boolean values for additionalProperties #1

merged 1 commit into from
Feb 3, 2021

Conversation

cyco130
Copy link

@cyco130 cyco130 commented Jan 6, 2021

The additionalProperties property of an object should be either a boolean value or a schema that additional object properties will be validated against. While trying out as-typed npm package as a potential replacement for json-schema-to-typescript package, I've noticed that as-typed doesn't currently support additionalProperties: false. This PR implements partial support for such usage.

Actually, the default value for additionalProperties should be true and json-schema-to-typescript types the following schema:

{
	type: "object",
	properties: {
		s: { type: "string" },
		n: { type: "number" },
	},
}

like this:

{
	s: string,
	n: number,
	[k: string]: any;
}

and removes the [k: string] key only if additionalProperties is false which is technically more correct. But implementing it this way breaks almost all tests so I believe what I've done is an acceptable middle ground for the time being.

@cyco130
Copy link
Author

cyco130 commented Jan 6, 2021

PS: I can fix the linting problems if you're interested in this idea at all. Awesome work BTW.

@lbguilherme
Copy link
Owner

Hi @cyco130, thanks for this.

I believe additionalProperties should default to false for us (as you did) even if it deviates from the specification. I agree with your changes as they are.

Could you fix the linter?

@cyco130
Copy link
Author

cyco130 commented Jan 21, 2021

@lbguilherme Thanks for the feedback. I will fix the linting errors as soon as I got the time (this weekend probably).

@lbguilherme lbguilherme merged commit ed501f6 into lbguilherme:master Feb 3, 2021
@lbguilherme
Copy link
Owner

@cyco130 Thanks for this. I ended up merging as-is and fixing with a followup commit.

@cyco130
Copy link
Author

cyco130 commented Feb 4, 2021

@lbguilherme Sorry I failed to finish it up before you did. Thank you for merging.

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 this pull request may close these issues.

2 participants