feat: Under-specified objects are Record<string, unknown>

[duncanbeevers]

Changes

This changes the fallback behavior when generating types for object schemas without specified properties to be Record<string, unknown> instead of Record<string, never>

This change likely constitutes a major version bump, but more-closely resembles the v5 behavior.
We could hide it behind a CLI flag for backwards compatibility.

Closes #1031

How to Review

The example snapshots are checked as part of the tests. Reviewing the changes to these snapshots is the best way to get a sense of the scope of the changes.

For many of the changes I also went back to the source OpenAPI yaml definitions to verify that Record<string, unknown> accurately represents the types returned from the actual API implementations.

Checklist

• Unit tests updated
• README updated (I'm already in there)
• examples/ directory updated (if applicable)

[duncanbeevers]

I've been using the 5.4.0 branch for quite some time to great effect.
In order to migrate to 6.1.x, I needed this functionality, and also #1022.

I published a private package with these changes and am using it happily in the existing app. (although it did reveal some places where I needed to make requestBody required.

[leppaott]

Upvoted as seems also what we need to migrate to v6.

[drwpow]

I’m not sure I agree with this change. Yes 5.x had that behavior, but this change does make the generated types for "type": "object" equivalent to "type": "object", "additionalProperties": true.

I guess the question is whether or not "additionalProperties": true is the default behavior or not. And that is the thing that changed from 5.x to 6.x.

I do think that in general, a clearer schema results in clearer generated types. If you wanted to generate Record<string, unknown>, I think "type": "object", "additionalProperties": true would be the best way for that. And that change should happen in the schema.

But in either case, I do like the flag idea. Rather than changing what 6.x does, we could add a --additional-properties=true flag to set the default behavior of objects that are missing that property. We could ship this behind a flag to not release a breaking version.

Edit: clarified some thoughts

[duncanbeevers]

I primarily based this change on schemas seen in the wild.

For example, on the Stripe API, a notoriously-well-documented schema, all those Record<string, never> values mean those parts of the API are not just unknown to TypeScript, but basically off-limits.

In my opinion, this proposed behavior better-matches the intent of real-world schema authors.

[duncanbeevers]

The --additional-properties flag already exists. However, using it causes the types to be widened across a huge number of objects, not just those where no specific schema is specified.

This change only affects the fallback behavior where no schema for an object is specified.

If there were an additional command-line flag, I'd imagine it would be something like --bare-objects-unknown.

[duncanbeevers]

@drwpow I've updated this PR to put the functionality behind the --empty-objects-unknown flag.

I tested the node-js generated pieces, but I didn't see a straightforward way to test CLI flag permutations against the example OpenAPI schema corpus.

I did go ahead and regenerate the examples in main, and opened this PR for it.
Then I used the "clean" base to verify the CLI flag behavior when regenerating the examples.

[mitchell-merry]

According to the OpenAPI docs:

"A free-form object (arbitrary property/value pairs) is defined as:

type: object

This is equivalent to

type: object
additionalProperties: true

and

type: object
additionalProperties: {}

"

so I believe Record<string, unknown> is the correct default behaviour here, and Record<string, never> when additionalProperties: false.

I think I disagree that a flag is necessary here. The correct behaviour is Record<string, unknown>, and this isn't a breaking change, since it doesn't widen/narrow any existing types.

[drwpow]

👍 Thanks for updating this example!

[drwpow]

@mitchell-merry ah thank you for providing that detail! Yes it does seem to be explicit in the Swagger 3.0 documentation. It’s much more ambiguous in the 3.1 specification.

I think I disagree that a flag is necessary here. The correct behaviour is Record<string, unknown>, and this isn't a breaking change, since it doesn't widen/narrow any existing types.

I’d still like for this to be a flag for now, because it would be a behavior change for 6.x of this library, and it would break types for people already using this version.

Because the discussion over generated types is stemming from ambiguity in an OpenAPI schema, we can could take 1 of 2 approaches: either err toward being too strict (you can’t access any properties that aren’t defined until you update your OpenAPI schema) or too lax (you can freely access any properties not in your OpenAPI schema by just asserting types). Just speaking personally, I‘d rather err on the side of being more strict, and requiring the OpenAPI schema to accurately describe all shapes rather than TypeScript. You’ll always get better mileage out of that approach, if you ask me!