Destructured parameters: flag.isOptional and reference types not working

[xAlien95]

• I have checked issues with bug label and found no duplicates

Expected Behavior

I'm using the command:

typedoc src --json exported.json --target es6

I expect the two arrow functions createAlarm and createAlarm2 to have the same json output (except for __namedParameters of course).

Actual Behavior

For createAlarm, Typedoc doesn't set the .isOptional flag to each parameter and outputs different types (e.g. repeat parameter type.elementType is directly an union instead of a reference, active parameter type contains true and false instead of boolean):

Steps to reproduce the bug

Run Typedoc on the following code:

type Frequency = (
  'Sunday'
  | 'Monday'
  | 'Tuesday'
  | 'Wednesday'
  | 'Thursday'
  | 'Friday'
  | 'Saturday'
);

type Switch = (
  'ON'
  | 'OFF'
);

/**
 * With destructured parameter
 *
 * - handles default values
 * - flag.isOptional not working
 * - reference type not working
 */
const createAlarm = (
  {
    /** The alarm active state */
    active = true,
    /** The alarm time */
    time = '',
    /** Whether the alarm should repeat on certain days */
    repeat = [],
  }: {
    active?: boolean | Switch,
    time?: string,
    repeat?: Frequency[],
  },
): string => `Alarm at ${time} every ${repeat.join(', ')} is ${active}`;

/**
 * Without destructured parameter
 *
 * - doesn't handle default values
 * - flag.isOptional works
 * - reference type works
 */
const createAlarm2 = (
  options: {
    /** The alarm active state */
    active?: boolean | Switch,
    /** The alarm time */
    time?: string,
    /** Whether the alarm should repeat on certain days */
    repeat?: Frequency[],
  },
): string => {
  const {
    active = true,
    time = '',
    repeat = [],
  } = options;

  return `Alarm at ${time} every ${repeat.join(', ')} is ${active}`;
};

Environment

• Typedoc version: 0.14.2
• TypeScript version: 3.2.4
• Node.js version: v10.13.0
• OS: macOS 10.14.2

[xAlien95]

I started looking for the code that handles function signatures reflection and I found these lines:

typedoc/src/lib/converter/factories/parameter.ts

Lines 29 to 34 in 6e77522

	if (_ts.isBindingPattern(node.name)) {
	parameter.type = context.converter.convertType(context, node.name);
	parameter.name = '__namedParameters';
	} else {
	parameter.type = context.converter.convertType(context, node.type, context.getTypeAtLocation(node));
	}

Adding the type parameter to .convertType() (and moving the comments in my crateAlarm() test function from the object parameter destructuring to the parameter type), I get the right output. For instance, here's the edited crateAlarm() function:

/**
 * With destructured parameter
 *
 * - handles default values
 * - flag.isOptional not working
 * - reference type not working
 */
const createAlarm = (
  {
    active = true,
    time = '',
    repeat = [],
  }: {
    /** The alarm active state */
    active?: boolean | Switch,
    /** The alarm time */
    time?: string,
    /** Whether the alarm should repeat on certain days */
    repeat?: Frequency[],
  },
): string => `Alarm at ${time} every ${repeat.join(', ')} is ${active}`;

Here's the edit I did in src/lib/converter/factories/parameter.ts#L29-L34:

if (_ts.isBindingPattern(node.name)) {
    parameter.type = context.converter.convertType(context, node.type, context.getTypeAtLocation(node));
    parameter.name = '__namedParameters';
} else {
    parameter.type = context.converter.convertType(context, node.type, context.getTypeAtLocation(node));
}

Here's the output:

Does _ts.isBindingPattern(node.name) returns true in circumstances other than the ones in which there's a __namedParameters parameter? Would be safe to replace those lines with the following?

parameter.type = context.converter.convertType(context, node.type, context.getTypeAtLocation(node));
if (_ts.isBindingPattern(node.name)) {
    parameter.name = '__namedParameters';
}

Thank you for your attention.

[aciccarello]

Thanks for investigating. I'll have to look more at this but it might help us with a few other parameter destructuring issues.

[avinashbot]

Is there any progress on this? Looks like the proposed fix would also handle #621.

One thing I'd like to add about the fix is that I'm not sure whether node.type would be present if the type is meant to be inferred from usage (e.g. ({ a, b, c }) => { ... }).

[Gerrit0]

A PR with a fix would be welcomed, I haven't had the time to look into this yet.

TS should yell at us if node.type isn't present since we have strict null checks on.

[Gerrit0]

This is fixed in 0.20 beta - I had to toss a couple exports on the test case, but the output for each signature is identical now.