[api-extractor] Fix an issue where destructured parameters were handled incorrectly

[octogonz]

Summary

Consider these functions:

export function printReport(title: string, {verbose}: {verbose:boolean}): void { 
  . . .
}

export function clearRange([min, max]: [min: number, max: number]): void { 
  . . .
}

API Extractor will incorrectly report the function parameter name as {verbose} or [min, max] which can sometimes cause an actual validation error when API Documenter was expecting a normal identifier.

Details

Discussed in this Zulip thread:

Comments in destructured parameters trigger errors

In a nutshell, the {verbose} string is:

• a local variable (implementation detail) that does not affect the public signature in any way that API consumers can detect
• awkward to handle in the docsite, which wants to present parameters as named objects
• problematic for other contexts like @param
• yet too ubiquitous to treat as an actual error, and probably required for some arcane type algebra scenarios

My proposed solution is to rewrite the signatures, synthesizing a conventional name for the anonymous parameter:

export function printReport(title: string, options: {verbose:boolean}): void { 
  . . .
}

export function clearRange(list: [min: number, max: number]): void { 
  . . .
}

The options and list names are used when there is exactly one destructured parameter. In more general cases (e.g. multiple such parameters), the naming pattern anonmyous, anonymous2, ... is used instead.

How it was tested

Impacted documentation

[octogonz]

Before merging, we can fix the ApiReportGenerator and DtsRollupGenerator as well. They are easier than ApiModelGenerator.

[ilg-ul]

The synthesised name options is probably right when added to optional parameters at the end of the list.

When there is a single set of destructured parameters, perhaps a more appropriate name would be params, instead of options.

[ilg-ul]

Does this patch also fixes the bug related to comments attached to destructured parameters?

[octogonz]

Does this patch also fixes the bug related to comments attached to destructured parameters?

Yes, if you look at the unit test, all comments get stripped during .d.ts emit except for one (// slash P3), which is in the subexpression that we are transforming.

[octogonz]

@ilg-ul After some discussion, we've decided to simplify the naming to input, input2, etc. (no longer using options or list).

I've also updated the implementation to apply to ApiReportGenerator. For now I will NOT apply this to DtsRollupGenerator, since that would require more extensive testing to be sure there are no regressions.

[ilg-ul]

Let me see if I understand it: if the function uses destructured parmeters, you display them as they were regular parameters? Can I add TSDoc @param comments for each of them and they will be rendered in the Description column for each parameter?

This matches my use case well, but what do you do if there are both regular and destructured parameters?

Does this apply only when the parameter types are defined inline, or it also applies when the types are declared via a separately declared object type? Since I'm considering updating my project to this solution.

Perhaps you should add a configuration option to enable this explicitly (something like promoteDestructuredParameters), otherwise you might break some projects.

FYI: look how badly is rendered such a case with the current version:

BTW, the @remarks section is rendered incorrectly, there is a link there that the documenter silently skipped.

The code is:

  /**
   * Constructs an actions collection instance.
   *
   * @remarks
   * The constructor performs partial initialisation. Complete initialisation
   * requires calling the {@link XpmActions.initialise} method.
   *
   * @param log - The logger instance for output and diagnostics.
   * @param engine - The Liquid templating engine for variable substitution.
   * @param substitutionsVariables - The variables available for substitution.
   * @param inheritedActionsMap - Optional map of actions inherited from a
   * parent package.
   * @param jsonActions - The JSON object containing action definitions, or
   * undefined if no actions are defined.
   * @param buildConfiguration - Optional build configuration this actions
   * collection belongs to.
   */
  constructor({
    log,
    engine,
    substitutionsVariables,
    inheritedActionsMap,
    jsonActions,
    buildConfiguration,
  }: {
    log: Logger
    engine: XpmLiquidEngine
    substitutionsVariables: XpmLiquidSubstitutionsVariables
    inheritedActionsMap?: Map<string, XpmAction>
    jsonActions: JsonActions | undefined
    buildConfiguration?: XpmBuildConfiguration
  }) {
    ...
  }

  /**
   * Completes the async initialisation of the actions collection.
   *
   * @remarks
   * This method implements the first step of lazy evaluation. It processes
   * all action definitions by expanding template action names based on matrix
   * parameters, but does not evaluate the action content or perform Liquid
   * substitutions. The actual template evaluation and variable substitution
   * occur later when individual actions are initialised via
   * {@link XpmAction.initialise}, and only for actions that are
   * actually used. This approach avoids unnecessary operations on unused
   * actions. The method also validates that all expanded action names are
   * unique.
   *
   * @returns A promise that resolves to `true` if initialisation was
   * performed, or `false` if already initialised.
   *
   * @throws {@link XpmError}
   * If duplicate action names are detected or if template expansion fails.
   */
  async initialise(): Promise<boolean> {
    ...
  }

The link in the initialise() method is rendered correctly.

[octogonz]

To be clear, intialise() has exactly one parameter. Before this PR, the parameter is anonymous. After this PR it is called input. The type of this parameter is a big inline expression.

It makes sense that you want "log" to get its own table row, but there are a few challenges:

• Then the name should be something like @param input.log. API Extractor and TSDoc I think accept this @param notation but don't really resolve it correctly; I forget the details.
• If we want to show the type of input.log in the docs, we have to extract it from that expression somehow.
• In TypeScript, as soon as we allow this, suddenly the type algebra will start to grow like a cancer - well why not initialise(title: string, [that stuff])? Why not { x: string } | { y: string } as the type of input? The possibilities are endless (Turing complete actually) so you have to carefully decide where to put the "moat" of what is/isn't allowed, to feel reasonable to users.

My goal in this PR was more humble: replace that anonymous subexpression with a name input because it is an implementation detail, not an actual part of the API signature.

[octogonz]

I will try to repro your missing hyperlink, but is probably an unrelated issue

[ilg-ul]

It makes sense that you want "log" to get its own table row, but there are a few challenges: ...

If you agree that it makes sense, can we find a solution to make something like this work?

If you think that the syntax I used in my code is unfortunate and you have a suggestion for a better, more standard syntax, I am ready to update my code.

I also tried a syntax like @param params.log, but I got warnings like (tsdoc-param-tag-with-invalid-name) The @param block should be followed by a valid parameter name: The identifier cannot non-word characters.

My personal feeling is that the current implementation (that silently ignores these @param names) is not ok. (And this is just another good reason why coupling api-documenter with api-extractor is an unfortunate design choice.)

I understand that for api-extractor usage this might be considered a single parameter, but for api-documenter, as for the function itself, these are separate parameters, and the expectation to use use multiple @param name definitions, and see them as separate rows in the Parameters table is not unrealistic.

What can we do?

How about the proposal to add promoteDestructuredParameters to api-extractor, so that api-documenter finds the parameter names?

[dmichon-msft]

I think TSDoc may have a gap for named sub-properties here; JSDoc handles it:
https://jsdoc.app/tags-param#parameters-with-properties

There are, in fact, observable runtime differences between a function that takes a single named parameter and one that destructures the same parameter: most notably the result of fn.toString() will be different, and there are tools (Playwright being a notable one) that make runtime decisions based on this.

[ilg-ul]

JSDoc handles it:

You mean this one?

/**
 * Assign the project to an employee.
 * @param {Object} employee - The employee who is responsible for the project.
 * @param {string} employee.name - The name of the employee.
 * @param {string} employee.department - The employee's department.
 */
Project.prototype.assign = function({ name, department }) {
    // ...
};

In other words, @params should define first a name for the entire object, since it is not present in the code, and then the destructured parameters as properties of this object?

It is fine with me.

This would also solve the dilema how to name the anonymous object (options, params, input, etc), since it passes the decision to the user.