json-schema-to-typescript icon indicating copy to clipboard operation
json-schema-to-typescript copied to clipboard

Schema definitions are generated multiple times when setting a property description

Open queengooborg opened this issue 3 years ago • 6 comments

This change was found somewhere between 10.1.5 and 11.0.1. When generating types, I am noticing that typedefs are generated twice in certain scenarios, particularly when referencing a schema definition.

Schema:

// This schema has been simplified to include only the relevant portions of the schema
{
  "$schema": "http://json-schema.org/schema#",

  "definitions": {
    /* ... */
    "status_block": {
      "type": "object",
      "properties": {
        "experimental": {
          "type": "boolean",
          "description": "A boolean value that indicates whether this functionality is intended to be an addition to the Web platform. Set to false, it means the functionality is mature, and no significant incompatible changes are expected in the future."
        },
        "standard_track": {
          "type": "boolean",
          "description": "A boolean value indicating whether the feature is part of an active specification or specification process."
        },
        "deprecated": {
          "type": "boolean",
          "description": "A boolean value that indicates whether the feature is no longer recommended. It might be removed in the future or might only be kept for compatibility purposes. Avoid using this functionality."
        }
      },
      "required": ["experimental", "standard_track", "deprecated"],
      "additionalProperties": false
    },

    "compat_statement": {
      "type": "object",
      "properties": {
        /* ... */
        "status": {
          "$ref": "#/definitions/status_block",
          "description": "An object containing information about the stability of the feature.",
          "tsType": "StatusBlock"
        }
      },
      /* ... */
    },
}

Before:

export interface CompatStatement {
  /* ... */
  /**
   * An object containing information about the stability of the feature.
   */
  status?: StatusBlock;
}

/**
 * This interface was referenced by `CompatDataFile`'s JSON-Schema
 * via the `definition` "status_block".
 */
export interface StatusBlock {
  /**
   * A boolean value that indicates whether this functionality is intended to be an addition to the Web platform. Set to false, it means the functionality is mature, and no significant incompatible changes are expected in the future.
   */
  experimental: boolean;
  /**
   * A boolean value indicating whether the feature is part of an active specification or specification process.
   */
  standard_track: boolean;
  /**
   * A boolean value that indicates whether the feature is no longer recommended. It might be removed in the future or might only be kept for compatibility purposes. Avoid using this functionality.
   */
  deprecated: boolean;
}

After:

/**
 * An object containing information about the stability of the feature.
 */
export type StatusBlock = StatusBlock;
export interface CompatStatement {
  /* ... */
  status?: StatusBlock;
}
/**
 * This interface was referenced by `CompatDataFile`'s JSON-Schema
 * via the `definition` "status_block".
 */
export interface StatusBlock1 {
  /**
   * A boolean value that indicates whether this functionality is intended to be an addition to the Web platform. Set to false, it means the functionality is mature, and no significant incompatible changes are expected in the future.
   */
  experimental: boolean;
  /**
   * A boolean value indicating whether the feature is part of an active specification or specification process.
   */
  standard_track: boolean;
  /**
   * A boolean value that indicates whether the feature is no longer recommended. It might be removed in the future or might only be kept for compatibility purposes. Avoid using this functionality.
   */
  deprecated: boolean;
}

queengooborg avatar Jul 07 '22 01:07 queengooborg

Thanks for the report! It's hard to help without a reproducible case. Can you please comment with a self-contained, reproducible example of the bug?

bcherny avatar Jul 11 '22 02:07 bcherny

Certainly -- here's the above example put into a runnable script to show the issue:

import { compile } from 'json-schema-to-typescript';

const opts = {
  bannerComment: '',
  unreachableDefinitions: true,
};

const schema = {
  $schema: 'http://json-schema.org/schema#',

  definitions: {
    status_block: {
      type: 'object',
      properties: {
        experimental: {
          type: 'boolean',
          description:
            'A boolean value that indicates whether this functionality is intended to be an addition to the Web platform. Set to false, it means the functionality is mature, and no significant incompatible changes are expected in the future.',
        },
        standard_track: {
          type: 'boolean',
          description:
            'A boolean value indicating whether the feature is part of an active specification or specification process.',
        },
        deprecated: {
          type: 'boolean',
          description:
            'A boolean value that indicates whether the feature is no longer recommended. It might be removed in the future or might only be kept for compatibility purposes. Avoid using this functionality.',
        },
      },
      required: ['experimental', 'standard_track', 'deprecated'],
      additionalProperties: false,
    },

    compat_statement: {
      type: 'object',
      properties: {
        status: {
          $ref: '#/definitions/status_block',
          description:
            'An object containing information about the stability of the feature.',
          tsType: 'StatusBlock',
        },
      },
    },
  },

  title: 'CompatDataFile',
  type: 'object',
  patternProperties: {
    '^__compat$': {
      $ref: '#/definitions/compat_statement',
    },
  },
};

console.log(await compile(schema, 'Schema', opts));

Note: if you comment out the line that says tsType: 'StatusBlock', then export type StatusBlock = StatusBlock; becomes export interface StatusBlock {...}, and the interface is still duplicated.

queengooborg avatar Jul 11 '22 05:07 queengooborg

Update: it seems that unreachableDefinitions: true causes this issue, as switching it to false will generate the expected typedefs. However, because we define custom tsTypes, disabling the option is not possible for us and removing the tsType is not an option either.

queengooborg avatar Jul 13 '22 13:07 queengooborg

Getting rid of tsType and moving description to the status_block definition fixes this. Would that work for you?

I'm not too familiar with the code, but it seems that when additional fields are present alongside $ref, then the property (e.g. your status property) is promoted as a unique interface as of 2ca6e50, e.g. StatusBlock1. Prior to that commit, it would get inlined instead of generating a new interface, and at that time tsType helped mask the issue.

edit: ^ btw, it appears this is due to @apidevtools/json-schema-ref-parser treating your $ref with additional fields as an "extended ref" and thus creates a new merged object as a shallow copy of the ref'd object plus the extra properties that are siblings to the $ref. Since they aren't referentially equal after running the resolver, json-schema-to-typescript sets different parents during its linking phase, and during code generation as of 2ca6e50 it becomes a new interface.

Outside of the quick fix I mentioned, just thinking out loud, I could envision a scenario where certain keywords, like description, are extracted into a separate tree prior to $ref resolution, and referenced during code generation to, for example, tack comments onto properties.

dhvector avatar Aug 01 '22 01:08 dhvector

Thanks for digging into this! Moving the description to the status_block definition did fix the issue. Unfortunately as a result, we lose the description on the status property itself (and thus lose IDE hints, which contributors do care about), but at least the TypeScript generates without duplicates. I even removed the custom tsType successfully!

I think that the potential solution of extracting properties prior to $ref resolution is a great one for a long-term fix. I can imagine other projects may not want to make this compromise, and it should greatly help reduce confusion during debugging!

queengooborg avatar Aug 01 '22 23:08 queengooborg