Always checks for required properties, even if they are missing from properties
#620
Conversation
Codecov Report
@@ Coverage Diff @@
## main #620 +/- ##
==========================================
- Coverage 93.81% 93.73% -0.08%
==========================================
Files 5 11 +6
Lines 291 527 +236
Branches 97 188 +91
==========================================
+ Hits 273 494 +221
- Misses 16 32 +16
+ Partials 2 1 -1
Continue to review full report at Codecov.
|
src/transform/schema.ts
Outdated
| } | ||
| let output = ""; | ||
| for (const r of missingRequired) { | ||
| output += `${r}: any;\n`; |
There was a problem hiding this comment.
| output += `${r}: any;\n`; | |
| output += `${r}: unknown;\n`; |
src/transform/schema.ts
Outdated
| @@ -45,6 +45,19 @@ export function transformSchemaObjMap(obj: Record<string, any>, options: Transfo | |||
| return output.replace(/\n+$/, "\n"); // replace repeat line endings with only one | |||
| } | |||
|
|
|||
| /** make sure all required fields exist **/ | |||
| export function addRequiredProps(properties: Record<string, any>, required: string[]): string[] { | |||
There was a problem hiding this comment.
This is a minor thing, but I’d recommend using Set<string> rather than string[], so that duplicates will automatically be removed for us. (e.g. required: ["foo", "foo", "foo", "bar"]). I don’t think it’s invalid to repeat properties in required, but the generated TypeScript will be incorrect if this happens (I know that this is on the schema author, but the error will be really weird so I’d rather just avoid it).
src/transform/schema.ts
Outdated
| // if empty object, then return generic map type | ||
| if ( | ||
| !isAnyOfOrOneOfOrAllOf && | ||
| (!node.properties || !Object.keys(node.properties).length) && | ||
| !node.additionalProperties | ||
| ) { | ||
| output += `{ ${readonly}[key: string]: any }`; | ||
| const emptyObj = `{ ${readonly}[key: string]: any }`; |
There was a problem hiding this comment.
Same here:
| const emptyObj = `{ ${readonly}[key: string]: any }`; | |
| const emptyObj = `{ ${readonly}[key: string]: unknown }`; |
There was a problem hiding this comment.
Thinking about this, this might be a breaking change for a lot of folks, who use empty projects and rely on the any. In my project, this wouldn't be a problem.
I will include this change for now. Let me know your thoughts
There was a problem hiding this comment.
I’m really sorry but I think you do raise a good point and you’ve changed my mind on this—this actually would cause disruption for people. And #554 didn’t really get a lot of support from people, so I’m starting to second-guess it. This PR shouldn’t be blocked by another that may or may not merge.
It seems there is another merge conflict (apologies); if you’re able to update that would you mind changing this back to any again? Again I’m sorry and this is 100% on me. If I don’t hear from you soon, I can update this PR myself and still give you credit for it. Consider this PR ready-to-go!
tests/schema.test.ts
Outdated
| @@ -9,7 +9,7 @@ const defaults = { | |||
| immutableTypes: false, | |||
| required: new Set<string>(), | |||
| rawSchema: false, | |||
| version: 3, | |||
| version: 3 | |||
There was a problem hiding this comment.
Not blocking for this PR, but if you could use the local Prettier settings for this project that’d cut down on some unnecessary diff here
| })`); | ||
| }); | ||
|
|
||
| it("empty object with required fields", () => { |
|
Requested a few minor changes; once those are in I’d be happy to merge |
|
Will address those later today. |
|
Updated the branch based on feedbaack |
|
fixed the merge conflict. |
|
@all-contributors please add @shuluster for code |
|
I've put up a pull request to add @shuluster! 🎉 |
Fixes #619