cdklabs / jsii-docgen

Generates reference documentation for jsii modules
Apache License 2.0
48 stars 11 forks source link

De-sugaring Type Aliases with arrays #1460

Open ReidWeb opened 3 weeks ago

ReidWeb commented 3 weeks ago

Having reviewed the docs it my understanding that JSII/JSII-Docgen de-sugars type aliases are de-sugared, this is a perfectly understandable design decision.

However I've come across a bit of a quirky behaviour as described below, and as is viewable in the minimum reproducible example repo I created.

/**
 * Interface for providing config for paramOne
 */
export interface IOne {
    /**
     * Param foo
     */
    readonly foo: string
}

/**
 * Declared type to wrap two options user can provide for the param
 */
export type ParamType = string | IOne

/**
 * Function parameters
 */
export interface FunctionParams {
    readonly paramOne: ParamType[]
}

export class ConstructFoo {

    readonly internalParam: ParamType[]

    constructor(fnParams: FunctionParams) {
        this.internalParam = fnParams.paramOne
    }
}

As you can see, I am declaring parameter paramOne to expect an array of objects, I am not explicitly stating the type of the array as would be the case if I declared string[] | IOne[], thus from a TypeScript perspective, it will allow users to pass a mixed array of strings and objects matching interface IOne, as is shown to be working in my example invocation.

Current Behaviour

However the docs come out as

```typescript
public readonly paramOne: string | IOne[];
```

Which implies only the second type in the union can be an array, if a user were to attempt to use this as described in the docs they would get invalid code.

const arrayParams : ParamType[]  = [ // ✅ Valid TypeScript Code
    {
      foo: 'bar'
    },
    'foobar'
]

const asDocsDescribe: string | IOne[]  = [ // 🚨 Not valid code!
    {
        foo: 'bar'
    },
    'foobar'
]

I believe this is an unintended consequence of the de-sugaring you are performing, and may require remediation?

Expected behaviour

Docgen should produce doc declaration for type paramOne as follows

```typescript
public readonly paramOne: string[] | IOne[];
```

Background

I am using this in a way where I want objects of interface IOne or strings in the interface, not both. I had declared my code as such as I found there were more optimal type guards available to me when I did it this way vs having string[] | IOne[]

mrgrain commented 1 week ago

Relevant part of the jsii assembly:

        {
          "abstract": true,
          "docs": {
            "stability": "stable"
          },
          "immutable": true,
          "locationInModule": {
            "filename": "lib/ConstructFoo.ts",
            "line": 21
          },
          "name": "paramOne",
          "type": {
            "collection": {
              "elementtype": {
                "union": {
                  "types": [
                    {
                      "primitive": "string"
                    },
                    {
                      "fqn": "jsii-docgen-mre.IOne"
                    }
                  ]
                }
              },
              "kind": "array"
            }
          }
        },
mrgrain commented 1 week ago

Docgen should produce doc declaration for type paramOne as follows

I'd even go further and say the type should be (which is what the jsii assembly says):

```typescript
public readonly paramOne: Array<string | IOne>;
```
ReidWeb commented 1 week ago

Yup, makes sense to me!

mrgrain commented 1 week ago

Labelling this as a p2 bug. jsii-docgen is a community led project with fixes from the maintainers mainly being provided as we need them for our own projects. This means it's unlikely we'll get to this issue anytime soon.

Contributions are welcomed and we aim to respond to all PRs as soon as possible.