Fix type regression in tsnext which throws TS2463

[voxpelli]

The tsnext tests are now failing: https://github.com/voxpelli/pony-cause/runs/7605089081?check_suite_focus=true

index.js(9,25): error TS2463: A binding pattern parameter cannot be optional in an implementation signature.

Happens at:

pony-cause/lib/error-with-cause.js

Lines 5 to 9 in 4009048

	/**
	* @param {string} message
	* @param {{ cause?: T }} [options]
	*/
	constructor (message, { cause } = {}) {

Likely caused by:

• fix(49869): No error in JavaScript on optional binding pattern parameter microsoft/TypeScript#50094
• No error in JavaScript on optional binding pattern parameter microsoft/TypeScript#49869

Spontaneous thought is that TS has regressed rather than this being a correct error, but have to dig into it.

[sandersn]

The TS equivalent, function test(everything? = '') { } is illegal:

TS1015 Parameter cannot have question mark and initializer.

In the JS below, options is marked as optional by the type system:

/**
 * @param {string} filled
 * @param {{ cause?: string }} options
 */
function test(filled, { cause } = {}) {
}

So the question is whether the duplicate marking of optionality should be legal in JS where it is illegal in TS. If it's illegal, then there needs to be an error for it same as in TS.

[voxpelli]

@sandersn I think its a bit harsh to error on duplicate marking of optionality, I myself quite like it and its not technically wrong.

I also think its a bit odd to allow:

  /**
   * @param {{ cause?: T }} [options]
   */
  constructor (options) {
    const { cause } = options || {};

But to ban:

  /**
   * @param {{ cause?: T }} [options]
   */
  constructor ({ cause } = {}) {

See this playground for an actual example of that.

I did check https://documentation.js.org/ and it will also infer that its optional if it has a default value, so in that way you are right, it is redundant, but I think it should be up to linters like eslint-plugin-jsdoc to point that out.

Also: The error message is not at all helpful in indicating that it is the redundant marking of optionality that is the issue here.

I think the fix of microsoft/TypeScript#49869 is good, but I don't think this additional (unintended?) change is good.

[jespertheend]

While I don't really mind whether the duplicate marking of optionality is legal or not, the fact that this breaks existing code is probably less than ideal.
The fix is rather easy though, in my project I've had to remove the optionality from the jsdoc comment in 23 instances, which I don't think is too much of a pain.

If this does end up staying illegal, I do think the error message will have to be changed as mentioned above. The current error is indeed not very helpful.

[voxpelli]

Yeah, I'm also discovering more and more as I go + now adding cron tests for catching these failures in TS early.

Here's another example of mine: https://github.com/voxpelli/node-pg-pubsub/blob/7b988008ca4935cc2b256e5f42afaca51461cd1a/index.js#L26-L31

[voxpelli]

To ensure that this doesn't get lost, I added a proper regression report to the TS-repo: microsoft/TypeScript#50286