Cannot run 'convert to named parameters' refactoring for method implementing an interface

[mjbvz]

TypeScript Version: 3.4.0-dev.20190311

Search Terms:

• refactor
• convert to named parameters

Code

For the TS:

interface IFoo {
    method(x: string, y: string): void;
}

const x: IFoo  = {
    method(x, y) {}
};

Place cursor in the parameters in method(x, y) and run the convert to named parameter refactoring

Bug:
Although the refactoring is listed, it fails to apply.

[RyanCavanaugh]

What's the expected diff in your opinion?

[mjbvz]

Ideally:

interface IFoo {
    method({x, y}: { x: string, y: string}): void;
}

const x: IFoo  = {
    method({x, y}) {}
};

While also changing other implementers of IFoo.

If that's too complicated to implement, can we detect that this is an interface implementation method and just not show the refactoring?

[mjbvz]

I am marking this as a priority since showing an error after you select a refactoring is not a good user experience.

Again, ideally we would support applying the refactoring but I'm not sure how practical it would be to implement. If that's not possible, let's see if we can detect this case and not show the refactoring to users at all.

[RyanCavanaugh]

Desired behavior: Don't show this refactoring if we can't apply it

[jessetrinity]

@mjbvz to clarify, is the refactor in question "Convert parameters to destructured object"?

[mjbvz]

Yes that's correct

[jessetrinity]

Found a couple more places where this fails to apply:

// abstract class
abstract class Foo {
  abstract foo(a: number, b: number): number;
}

class Bar extends Foo {
  foo(a: number, b: number): number { return a + b; }
}

// Import declaration conflicts with local declaration of 'bar'.ts(2440)
// note: we shouldn't even try here, don't show the refactor since there is a conflict with an error
import { foo } from "./foo";
function foo(a: number, b: number) {
  return a + b;
}
foo(1,2);

There are likely other cases because the getApplicableRefactors step only checks that the function declarations exist. The best path forward is to add a verification check there so we don't offer not implemented cases, then knock out the individual cases.