search

ostrowr / ts-json-validator

Let JSON play nicely with Typescript
MIT License
342 stars 7 forks source link

Add tests for additionalProperties #30

Closed bigpopakap closed 4 years ago

bigpopakap commented 4 years ago

I like to be able to set additionalProperties: false in schemas so that Ajv's removeAdditional: true option (enabled in #29) can strip out extra properties when a schema says so. (Here are the docs about the removeAdditional option)

bigpopakap commented 4 years ago

I broke this out from #29 as requested here. Unfortunately, it's a little confusing right now because this PR includes the commits from #29 since the test I need to be able to pass options to ajv

UPDATE: rebased/synced this PR so now it just includes the single relevant commit

ostrowr commented 4 years ago

I haven't checked this out and tried it yet, so I might be misunderstanding the intended use case here – but boolean should be assignable to SchemaLike. I think just writing S(false) rather than false will accomplish what you're looking to do here without having to widen the type of additionalProperties.

bigpopakap commented 4 years ago

Oh wow, yes, I also just noticed #26! I must have tried this before that change landed, and hadn't tested it again since then. That looks like it'll work. I'll give it a shot sometime today, and close this PR if it's been solved.

bigpopakap commented 4 years ago

I just push some updates. It still isn't completely done (sorry for the noise), but I think this PR gets a little closer to a solution now. I'll clean up the tests sometime soon, and will push another version that actually proposes a full solution.

bigpopakap commented 4 years ago

Ok, I think this PR is in its final state. I don't think it's possible to do any better than is already there. I added some tests for some expected behavior. Some assertions are commented out and reference a Typescript issue that's blocking them, mostly for documentation. If you find these tests useful, we can merge this PR. Otherwise, I don't think there is any feature improvement I can make here.

Thank you for all your time and attention!

ostrowr commented 4 years ago

OK, this all makes sense! This type system isn't quite sound but is hopefully still useful for your use-case.

I'll go ahead and merge these new tests!

one thing to note is that the expectType function isn't actually that useful – if the thing you're testing is assignable to the thing you're expecting, then it will return true even if they're not exactly equal. I have an idea about how to improve this and will push that up shortly, then cut a release so everyone can use the ajv properties.

bigpopakap commented 4 years ago

Thank you @ostrowr!

Yeah, it'll be great to get a new version so I can start using the ajv properties. And I'm glad you found these tests useful.

The type system is actually quite great! I think this whole project is super useful and well done. Having this simple Typescript wrapper around JSON schemas is more than enough for my use cases. I understand that not all JSON schema features will be replicable in Typescript, but you've documented all of them really well in the README. And in this particular case, it seems like we're limited by Typescript itself.

I look forward to seeing what you do about the expectType function! As I was writing these tests, it did occur to me that, like you say, something like expectType<unknown> is not very useful :P Would you mind tagging me in a PR or commit so I can see what you do (just for my own learning)?

ostrowr commented 4 years ago

I look forward to seeing what you do about the expectType function! As I was writing these tests, it did occur to me that, like you say, something like expectType is not very useful :P Would you mind tagging me in a PR or commit so I can see what you do (just for my own learning)?

@bigpopakap I'm not sure if I'm going to merge this, because working on the branch it's pretty clear that I'm going to have to write some crazy types to get tests to pass typechecking, but https://github.com/ostrowr/ts-json-validator/compare/expect-true is the basic idea. Same equality relation I use in https://ostro.ws/2019/12/09/taking-types-too-far/