Support JSDoc param destructuring

[nmussy]

🚀 Feature Request

Affected Languages

• TypeScript or Javascript
• Python
• Java
• .NET (C#, F#, ...)

General Information

• JSII Version: 0.18.0
• Platform: all

• I may be able to implement this feature request

• This feature might incur a breaking change

Description

JSII doesn't allow JSDoc-ing destructured parameters. For example:

/**
 * @param bar Example description
 */
public foo({ a, b }: {[key: string]: string}) {
    console.log(a, b);
}

throws the following error:

warning TS9999: JSII: In doc block of 'foo', '@param bar' refers to a nonexistent parameter.

In a real-world use:

/**
 * Generate an Domain Name health check
 *
 * @param scope the parent Construct for this Construct
 * @param id the logical name of this Construct
 * @param props Domain name health check properties
 */
public static domainName(
    scope: Construct,
    id: string,
    { protocol, ...props }: DomainNameHealthCheckProps
) {
    return new EndpointHealthCheck(scope, id, { ...props, ...protocol.options });
}

Proposed Solution

Don't check the parameter name for destructured properties

[RomainMuller]

The restructuring is syntax sugar in TypeScript/Javascript that doesn't quite translate in other languages (such as Java), where we wouldn't know what to attach such documentation to.

I would actually be somewhat more inclined to prohibit usage of the destructuring syntax in exported declarations...

[nmussy]

I'm not sure I understand. What would be the difference in translating to other languages my previous example vs. the following?

/**
 * Generate an Domain Name health check
 *
 * @param scope the parent Construct for this Construct
 * @param id the logical name of this Construct
 * @param props Domain name health check properties
 */
public static domainName(
    scope: Construct,
    id: string,
    props: DomainNameHealthCheckProps
) {
    const { protocol, ...otherProps } = props;
    return new EndpointHealthCheck(scope, id, { ...otherProps, ...protocol.options });
}

[RomainMuller]

Let's take the case of your first example:

/**
 * @param bar Example description
 */
public foo({ a, b }: {[key: string]: string}) {
    console.log(a, b);
}

The doc-string mentions something called bar, but there is no declaration named bar.

The example you propose with the de-structuring done inside the method as opposed to in the signature gives me an absolutely unambiguous property name label, which I can translate to other languages as-is... But it's not possible to infer the label in other scenarios.

[nmussy]

I'm probably missing something here, but couldn't you use the @param label? They would have to be ordered if there are multiple argument destructred:

/**
 * @param bar Probably the first argument
 * @param qux Definitely the second argument
 * @param baz Probably the third argument
 */
public foo({ a, b }: {[key: string]: string}, qux: string, { c }: {[key: string]: string}) {
    console.log(a, b, qux, c);
}

This is currently how IDEs handle it:

[RomainMuller]

Well that's just not how JSDoc works... The label after @param must correspond to the name of an argument in the function's signature... I don't want to go guessing because it may not always be correct.

[nmussy]

JSDoc has a section on "Documenting a destructuring parameter", but it doesn't account for multiple parameters.

If a parameter is destructured without an explicit name, you can give the object an appropriate one and document its properties.

[RomainMuller]

That still makes the interpretation of the documentation non-deterministic. I don't want to depend on ordering of declarations for entities that include a name that should map to other declarations. That seems backwards to me & is a recipe for shipping incorrect documentation.

Also, apparently, TSDoc (which is a variation of JSDoc that is slightly more appropriate, since TypeScript) has this rule: microsoft/tsdoc#186 -- it'd recommend a syntax such as:

/**
 * doc for `a`,`b`, and `c` here:
 * @param a - the first parameter
 * @param b - the second parameter
 * @param c - the third parameter
 * @returns the return value
 */
function foo({
  a,
  b,
  c
}:{
  a: string,
  b: number,
  c: string
}): boolean {

}

While this form makes sense to me, that wouldn't be something the can be ported over to other languages easily.

[RomainMuller]

Let's flip the conversation around if you will (because I just feel like we're going in circles here). What problem are we trying to solve here?

Fundamentally, we're just trying to unblock a short-hand syntax for destructuring which is a minor code change away. That pattern is a very natural thing to do in Javascript, but is impossible to model in at least some other target languages.

Purely from an API design standpoint (talking about the API that is compiled by jsii here), I see two main scenarios:

• { a, b } = { [key: string]: string } - this makes for a relatively poor API contract
  • the signature implies only a and b will be considered, but the declared type of the parameter does not even mention the need for an a and b fields
  • I also reckon TypeScript will infer the types of a and b to be string, but they'll actually be undefined if not provided through arguments, which can lead to bugs that tsc could have avoided.
• { a, b } = SomeTypeName - this is sugar for when the implementation only cares for a subset of SomeTypeName's features
  • should there be a parent interface/class to represent only that subset? In other words - is the method's contract too broad?
  • otherwise, this method is only leaking implementation details through it's signature

Basically, I understand (I think) the papercut here, but I'm not sure the benefits of the de-structured notations are worth the added complexity (and non-determinism) of our documentation parsing.

[nmussy]

Completely forgot that TSDoc existed. This syntax is cute, but not really useful for the CDK, since we're already making and documenting interfaces for everything.

And yeah, I agree that relying on parameter order is less than idea, but it seems that it's what TSDoc is doing, without relying on the parameter name. In the TSDoc Playgound:

/**
 * @param second - This should be the second param
 * @param first - This should be the first param
 * @returns 0
 */
function getZero(first, second) {
    return 0;
}

[nmussy]

Like you've mentioned, this is a very minor paper cut. The syntactic sugar that parameter destructuring provides is 1 line per parameter. We can close this issue.

On a sidenote:

I also reckon TypeScript will infer the types of a and b to be string, but they'll actually be undefined if not provided through arguments, which can lead to bugs that tsc could have avoided.

That's kind of unfortunate, I would have hoped them to be typed string | undefined.