fix: correctly transform untyped nullable field

[davejsdev]

Changes

What does this PR change? Link to any related issue(s).

Fixes #1407

Correctly transform untyped nullable field to unknown instead of Record<string, unknown> | null

How to Review

How can a reviewer review your changes? What should be kept in mind for this review?

First PR, so are there any test cases I'm missing? Anything I should add? Anything I should keep in mind going forward?

Checklist

• Unit tests updated
• docs/ updated (if necessary)
• pnpm run update:examples run (only applicable for openapi-typescript)

[changeset-bot]

🦋 Changeset detected

Latest commit: 282ad78

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openapi-typescript	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[davejsdev]

The issue suggested the type should be unknown | null, but unknown is effectively the same, and tsUnionOf() won't do a union if the type is already unknown; it just returns unknown.

[drwpow]

👍 this was really old code that someone argued that “object types” were the default type, but I couldn’t find anything in the spec suggesting that. Anyways, this is a good change to make.

[drwpow]

Ah tests failed for the gigantic list of snapshots. You’ll have to run the update snapshots script and commit the changes (which is also a helpful sanity check)

[davejsdev]

Ah tests failed for the gigantic list of snapshots. You’ll have to run the update snapshots script and commit the changes (which is also a helpful sanity check)

Ahh, I wasn't watching and rebuilding when I made the changes, so the snapshot script was referencing an old build without the changes.

CI catches it but wondering if it's worth running a build as part of the update:examples script to ensure it's always running with the most recent changes. The tradeoff is it's redundant if your build is already up to date. It's fast though so 🤷

I can add it in a new PR if that tradeoff makes sense.

[drwpow]

CI catches it but wondering if it's worth running a build as part of the update:examples script to ensure it's always running with the most recent changes. The tradeoff is it's redundant if your build is already up to date. It's fast though so 🤷

Oh yup that’s a great idea! And yeah build only takes a sec so that could happen.

Also the update script is a little janky/race condition-y so feel free to improve that however you feel like. That was just a quick slap-together.

[drwpow]

Nice! Much better

[drwpow]

I don’t like snapshot tests as your only tests. But as a sanity check, and comparing to “real world” schemas to see what they do, I find them really helpful. People do some silly things in actual schemas 😛

[drwpow]

Looks great, thank you!

Since the main branch is currently the v7 beta, I’ll release this manually on 6.x. I’ll do that until v7 is stable, then changesets creates release PRs that just need merging.

There may be a way to set that up with changesets, but also changesets has some existing bugs with how it versions monorepos so I also don’t trust it sometimes (e.g. since openapi-fetch depends on it, it gets confused and may bump a major version on that, too). But either way, it’s getting closer to v7 being stable so it’s just easier to do manually for a little longer.

[WickyNilliams]

Hey, this change has caused some issues for us

Consider the changes to the digitial ocean snapshot (i think i'm marrying the correct part of the schema to the correct part of the snapshot). Their schema looks like this:

    anyOf:
      - title: 'null'
        type: object
        nullable: true
        description: If the floating IP is not assigned to a Droplet, the
          value will be null.
      - $ref: '../../droplets/models/droplet.yml'

This PR resulted in this change to the snapshot:

-    droplet?: (Record<string, unknown> | null) | external["resources/droplets/models/droplet.yml"];
+    droplet?: unknown;

I feel that unknown here loses some important type info. If i were reading the DO schema and building a mental model, I would expect: a (nullable) object OR the droplet model. So it feels as though the removed line was correct and a better representation of the schema.

We have the exact same construct throughout our schema: a (nullable) object OR [some model]. After this change, those types are now unknown, causing errors in previously working code.

When slightly modifying the schema given in the original issue (#1407), to include type: object:

openapi: 3.0.0
info:
  title: example
  version: 0.1.0

paths:
  /my_route:
    get:
      responses:
        '200':
          description: 'Description'
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Test'

components:
  schemas:
    Test:
      type: object
      properties:
        nullable_object_field:
          type: object
          nullable: true

The following output is produced:

Test: {
  nullable_object_field?: unknown;
};

This does not seem correct. But using type: number and nullable:

nullable_number_field:
  type: number
  nullable: true

The following output is produced:

Test: {
  nullable_number_field?: number | null;
};

Which is correct. So I guess the logic may need refining? I think it makes sense to have a blanket unknown where there is no type specified, but it should be if and only if no type is specified

[WickyNilliams]

A possible fix might be:

if (schemaObject.nullable) {
  if ("type" in schemaObject) {
    finalType = tsUnionOf(finalType || "Record<string, unknown>", "null");
  } else {
    finalType = tsUnionOf(finalType || "unknown", "null");
  }
}

So using the old logic if there is a type , and the new logic otherwise?

[WickyNilliams]

@davidleger95 just in case you missed the above comments. I'd be interested to hear your thoughts