Broken generation with additionalProperties

[isaacl]

> tsc --strictNullChecks Broken.d.ts
Broken.d.ts:11:3 - error TS2411: Property 'message' of type 'string | undefined' is not assignable to string index type 'string'.

Generated via v10.0.0:

...
export interface FormObject {
  message?: string;
  [k: string]: string;
}

Schema

{
  "allOf": [{ "$ref": "#/definitions/FormObject" }],
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "FormObject": {
      "properties": {
        "message": {
          "type": "string",
          "maxLength": 4000
        }
      },
      "additionalProperties": {
        "type": "string",
        "maxLength": 100
      },
      "type": "object"
    }
  }
}

I left the maxLength properties in so you can see why I wrote the schema like this.

[isaacl]

From ts handbook

While string index signatures are a powerful way to describe the “dictionary” pattern, they also enforce that all properties match their return type. This is because a string index declares that obj.property is also available as obj["property"].

[isaacl]

So this is more broken than I thought. Basically the index type in typescript enforces on all existing properties, while in json additionalProperties spec only applies to things not listed in properties.

[isaacl]

additionalProperties probably needs to always translate to

[k: string]: unknown;

[bcherny]

This is a bug, and we should find a better way to handle this case.

One option is to always emit index signatures as [k: string]: T | undefined. In your case, [k: string]: string | undefined would fix the TS error.

However, I'm not sure this is always desirable, and if it wasn't for the message property, this may impose an unreasonable burden on consumers, who then need to check every index property for undefined (note: if a consumer is using the --noUncheckedIndexedAccess TSC flag this may be what they want, but it's a burned for those consumers that don't use the flag).

A better approach may be to change the way we emit index signatures, to always emit them as an intersection type:

export type FormObject = {[k: string]: string} & {message?: string}

declare const x: FormObject

x.a.toUpperCase() // OK
x.message.toUpperCase() // Error: Object is possibly 'undefined'

I'd love input from others. Are there cases this might not handle? Examples welcome.

[isaacl]

Unfortunately it's also broken for required properties that simply have a different type. This is invalid because the index type must contain the union of all listed types. So really I think the only option is to put the index type as 'unknown'.

...
export interface FormObject {
  message: number;
  [k: string]: string;
}

generated by:

{
  "allOf": [{ "$ref": "#/definitions/FormObject" }],
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "FormObject": {
      "properties": {
        "message": {
          "type": "number"
        }
      },
      "required": ["message"],
      "additionalProperties": {
        "type": "string"
      },
      "type": "object"
    }
  }
}

[isaacl]

Yeah intersection might work

[G-Rath]

This is a bug, and we should find a better way to handle this case.

The error in the body of the original issue is a duplicate of #235 which was resolved by implementing --strictIndexSignatures.

(which I agree should ideally be on by default).

However, I'm not sure this is always desirable, and if it wasn't for the message property, this may impose an unreasonable burden on consumers, who then need to check every index property for undefined

This should be desirable as IMO it gives the more accurate type - while noUncheckedIndexedAccess now does effectively add this, it applies to more than just this case (i.e array index access).

If a set of properties are known on the object, then there are a few ways of handling that - most powerfully you can use declaration merging, which is how we handle this over at DefinitelyTyped i.e for process.env:

declare global {
  export namespace NodeJS {
    export interface ProcessEnv {
      // eslint-disable-next-line @typescript-eslint/naming-convention
      GITHUB_WEBHOOK_SECRET: string;
    }
  }
}

This doesn't require the global or namespace, so using your example:

export type Demo = FormObject;

export interface FormObject {
  [k: string]: string | undefined;
}

export interface FormObject {
  message: string;
}

const fn = (demo: Demo) => {
  console.log(demo.message.toUpperCase()); // fine

  if(demo.body) {
    console.log(demo.body.toUpperCase()); // fine
  }

  console.log(demo.header.toUpperCase()); // error!
}

playground link.

emit them as an intersection type:
Are there cases this might not handle?

Yes, it prevents declaration merging as that can only be done with interfaces. This is why in DefinitelyTyped we tend to use empty interfaces (or interfaces with an index) as it lets you do cool things in apps like defining known headers for webhook lambdas:

declare module 'aws-lambda' {
  export interface APIGatewayProxyEventHeaders {
    'X-GitHub-Event': GithubEventName;
    'X-GitHub-Delivery': string;
    'X-Hub-Signature': string;
    'X-Hub-Signature-256': string;
  }
}

[fredericbarthelet]

Facing the same issue for the patternProperties JSON schema keyword where definitions does not match.

{
  "allOf": [{ "$ref": "#/definitions/FormObject" }],
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "FormObject": {
      "properties": {
        "message": {
          "type": "number"
        }
      },
      "required": ["message"],
      "patternProperties": {
        "^[a-zA-Z0-9]{1,255}$": {
          "type": "string"
        }
      },
      "type": "object"
    }
  }
}

is generating

export interface FormObject {
  message: number;
  /**
   * This interface was referenced by `undefined`'s JSON-Schema definition
   * via the `patternProperty` "^[a-zA-Z0-9]{1,255}$".
   */
  [k: string]: string;
}

However, using union types rather than intersection seems to do the trick. Am I missing something or would you consider treating additionalProperties/patternProperties as a union like shown below ?

export type FormObject =
  | { message: number; }
  /**
   * This interface was referenced by `undefined`'s JSON-Schema definition
   * via the `patternProperty` "^[a-zA-Z0-9]{1,255}$".
   */
  | { [k: string]: string; }

[jeroenpelgrims]

However, using union types rather than intersection seems to do the trick. Am I missing something or would you consider treating additionalProperties/patternProperties as a union like shown below ?

export type FormObject =
  | { message: number; }
  /**
   * This interface was referenced by `undefined`'s JSON-Schema definition
   * via the `patternProperty` "^[a-zA-Z0-9]{1,255}$".
   */
  | { [k: string]: string; }

That type is not representative for the JSON schema though, no? (I'm new to JSON schema so I might be interpreting it wrong)
As I understand the JSON Schema defines the object to have a property message of type number, AND additional properties with keys that match the pattern and that have values of type string.
This union type in your code defines an object that has a property message of type number OR has properties with a value of type string.
An intersect type would correctly define the type as having both a message property of type number AND additional properties with type string.

[fabrykowski]

Hi! Any chance of reviewing PR #383 which might close this issue?