Convert to named parameters #30089

gabritto · 2019-02-25T22:36:08Z

Convert to "named parameters" refactor

A function-like declaration with parameters is refactored to receive a single parameter object with properties corresponding to its old parameters. Thus a function's arguments can be named when calling it. This applies to functions, methods, constructors, arrow functions and function expressions.

Example:

function add(a: number, b: number, c: number): number {
    return a + b + c;
}

would be refactored to

function add({ a, b, c }: { a: number, b: number, c: number }): number {
    return a + b + c;
}

Refactor availability

The refactor will be listed as available on:

functions
methods
constructors for class declarations that have names or for class expressions which are assigned to a const variable without a type annotation
arrow functions and function expressions that initialize a const variable without type annotation
The refactor will not be available on:
overloads
functions with less than 2 parameters
functions having binding patterns in its parameter declarations
constructors with parameters that have properties, e.g.

class Foo {
  constructor(private a: string) {}
}

References

This refactor changes a function's signature, so it affects the way a function should be called either directly or indirectly. The refactor also changes assignability to and from a function's type, since its type will change.
Because of that, there are several situations where performing the refactor could cause a breaking change that would in the best scenario result in type errors or, in the worst scenario, runtime errors and wrong behavior.
The current solution to this problem is to adopt a conservative approach: the refactor will perform changes only if those changes are guaranteed not to break the code. Otherwise, it will not perform any change.
The refactor has to look at all the references of a function (through findAllReferences) to determine if changing it is safe. If a constructor is being refactored, the refactor also looks at all the references of the parent class declaration or class expression.
However, finding all references is too slow to be performed when the user is checking what are the available refactors. So the refactor will only look at all references if the user tries to apply it.
Therefore, the refactor can fail to perform changes even if it was listed as available and the user tried to applied it. Currently there is no way to inform a user why a refactor has failed.

Future Work: create a mechanism for informing the user why a refactor failed.
Related issue: Extend tsserver interface to allow refactoring failure reporting #28410

Good cases

Direct calls

The refactor can change direct calls to functions to conform to the new function signature. This is done by taking the old argument list and replacing it with an argument object whose properties have values corresponding to the old arguments.
Example:
1.

function foo(a: number, b: number) { }
foo(1, 2);

becomes

function foo({ a, b }: { a: number; b: number; }) { }
foo({ a: 1, b: 2 });

const foo = function(a: number, b: number) { };
foo(1, 2);

becomes

const foo = function({ a, b }: { a: number; b: number; }) { };
foo({ a: 1, b: 2 });

class Foo {
    bar(t: string, s: string): string { }
}
var foo = new Foo();
foo["bar"]("a", "b");
foo.bar("a", "b");

becomes

class Foo {
    bar({ t, s }: { t: string; s: string; }): string {
        return s + t;
    }
}
var foo = new Foo();
foo["bar"]({ t: "a", s: "b" });
foo.bar({ t: "a", s: "b" });

Imports & Exports

Since findAllReferences can track imports/exports across a project, importing/exporting the refactored function should be allowed.

Future Work: recognize import/export references.
Update: done in Handle imports and exports in 'convert parameters to destructured object' #30475.

Other good cases

Future Work: think about what other usages of the refactored function should be allowed.

Good class cases

If a constructor is being refactored, the refactor will also check if all the references to the corresponding class are valid usages.
Currently, for a class declaration (class C { ... }), the following usages are recognized:

New calls:

class C { }

const c = new C();

Property access and element access:

class C {
    static a: number = 2;
}

const b = C.a;
C["a"] = 3;

Type references and heritage clauses

class C { }
let c: C;
function f(c: C) { }
class B extends C { }
class A implements C { }

For a class expression (const c = class { ... }), the following usages are recognized:

New calls:

const c = class C { }

const a = new c();

Property access and element access:

const c = class C {
    static a: number = 2;
}

const b = c.a;
c["a"] = 3;

JSDoc

Currently the refactor will not do anything with JSDoc, it will treat it as regular comments.

Non-inline JSDoc

Example:

/**
 * @param name person's name
 * @param address person's address
 */
function getPerson(name: string, address: string) { }

should become something like

/**
 * @param {object} obj parameter object
 * @param obj.name person's name
 * @param obj.address person's address
 */
function getPerson({ name, address }: { name: string, address: string }) { return name + address; }

Future Work: non-inline JSDoc on the refactored function should be reorganized as above to reflect the parameter change.
There are also issues (JSDoc support for destructured parameters #11859, JSDoc comment for destructuring param: description text not displayed #24746) that should be resolved to support the JSDoc syntax with destructuring in both JS and TS.

Inline JSDoc

Example:

function getPerson(/** person's name*/ name: string, /** person's address*/ address: string) { }

Since the parameter names above will become binding elements in a destructuring pattern, we cannot inline the existing JSDoc,
so the refactor will break inline JSDoc.

Future Work: think about how the refactor should behave in this case.
Question: Should it just break the JSDoc? Try to convert it to non-inline JSDoc?

Rest parameters

The refactor works for rest parameters as illustrated below. The rest parameter will be converted to an optional property of the new parameter object, and the corresponding variable will be initialized to an empty array. This ensures that the variable is not undefined in case no rest arguments are provided, which reflects how rest parameters behave.

Example:

function buildName(firstName: string, middleName: string, ...restOfName: string[]) {
    return firstName + " " + middleName + " " + restOfName.join(" ");
}

let employeeName = buildName("Joseph", "Samuel", "Lucas", "MacKinzie");

becomes

function buildName({ firstName, middleName, restOfName = [] }: { firstName: string, middleName: string, restOfName?: string[] }) {
    return firstName + " " + middleName + " " + restOfName.join(" ");
}

let employeeName = buildName({ firstName: "Joseph", middleName: "Samuel", restOfName: ["Lucas", "MacKinzie"]);

This parameter

A function can have a this parameter as its first parameter. In this case, the refactor should ignore this parameter and only modify the other parameters.

Example:

function f(this: void, a: string, b: string) { }
f("a", "b");

becomes

function f(this: void, { a, b }: { a: string, b: string }) { }
f({ a: "a", b: "b" });

Parameters with initializers

A parameter with an initializer is considered an optional member of the new parameter type. If a parameter has an initializer but has no type annotation, its type is inferred (using checker.getTypeAtLocation(node)).

Example:

function add(a: number, b = 1) {
    return a + b;
}

should be

function add({ a, b = 1 }: { a: number, b?: number }) {
    return a + b;
}

Optional parameters

When a parameter is optional (when it is followed by a ? or has an initializer), the corresponding property in the generated parameter object type should also be optional.
In addition, if all parameters are optional, the new parameter object should be optional. This is achieved by adding an empty object initializer.

Example:

function foo(a: number, b?: number) { return a; }
foo(1);

becomes

function foo({ a, b }: { a: number; b?: number; }) { return a; }
foo({ a: 1 });

and

function bar(a?: string, b?: string) { }
bar();

becomes

function bar({ a, b }: { a?: string; b?: string; } = {}) { }
bar();

JavaScript

Currently the refactor is not available on a JavaScript file, but we would like the refactor to work on JavaScript as well. Essentially the difference in the refactor would be the omission of type annotations.

Example:

function foo(a, b) { return a + b; }

becomes

function foo({ a, b }) { return a + b; }

JS code can be harder to analyze and to validate the refactor's changes.
So, if the refactor does something wrong, it would be harder for the user to notice.
If the refactored function has annotations, it becomes easy to apply the refactor.

Some bad cases

Aliasing

As shown above, aliasing makes it not possible to track down all calls and usages of the refactored function.

Example:

const foo = (a: number, b: number) => a + b;
foo(1, 2);
var otherFoo = foo;
otherFoo(1, 2);

after the refactor:

const foo = ({ a, b }: { a: number; b: number; }) => a + b;
foo({ a: 1, b: 2 });
var otherFoo = foo;
otherFoo(1, 2); // Error: Expected 1 arguments, but got 2.

Other example:

class Foo {
    constructor(s: string, t: string) { }
}
var c = Foo;
var f = new c("a", "b");

after refactoring the constructor:

class Foo {
    constructor({ s, t }: { s: string; t: string; }) { }
}
var c = Foo;
var f = new c("a", "b"); // Error: Expected 1 arguments, but got 2.

Class expression example:

const foo = class {
    constructor(a: number, b: number) { }
}
class Bar extends foo {
    constructor() {
        super(1, 2);
    }
}

after refactoring the constructor:

const foo = class {
    constructor({ a, b }: { a: number; b: number; }) { }
}
class Bar extends foo {
    constructor() {
        super(1, 2); // Error: Expected 1 arguments, but got 2.
    }
}

Types

Since the refactor changes the type of the function, there might be type errors.
Example:

function add(a: number, b: number) { return a + b; }
function sum(fn: (a: number, b: number) => number, ...numbers: number[]) {
    let total = 0;
    for (let n of numbers) {
        total += n;
    }
    return total;
}
var x: (a: number, b: number) => number = add;
var y = sum(add, 1, 2, 3);

becomes:

function add({ a, b }: { a: number, b: number }) { return a + b; }
function sum(fn: (a: number, b: number) => number, ...numbers: number[]) {
    let total = 0;
    for (let n of numbers) {
        total += n;
    }
    return total;
}
var x: (a: number, b: number) => number = add; // error on x: type ... is not assignable to type ...
var y = sum(add, 1, 2, 3); // error on add: argument of type ... is not assignable to parameter of type ...

Nameless declarations (anonymous functions/classes)

Anonymous functions and classes are tricky when looking for references.
Example:

var foo = (a: number, b: number) => a + b;
foo(1, 2);
// ... more code ...
foo = (a: number, b: number) => a - b; 
foo(1, 2);

In the example above, if we refactor the arrow function assigned to foo on line 1, then we should refactor the call on line 2. But later on foo is reassigned, so this will cause a type error. The same goes for constructors of class expressions.

`bind`/`call`/`apply` calls

Calling bind, call or apply on a function that has been refactored might go wrong.
Example:

function add(a: number, b: number, c: number) { return a + b + c; }
let add1 = add.bind(null, 1);
let z = add1(2, 3);
let x = add.call(null, 1, 2, 3);
let y = add.apply(null, [1, 2, 3]);

becomes

function add({ a, b, c }: { a: number, b: number, c: number }) { return a + b + c; }
let add1 = add.bind(null, 1);
let z = add1(2, 3); // NaN or Argument not assignable to parameter
let x = add.call(null, 1, 2, 3); // NaN or Expected 2 arguments, but got 4
let y = add.apply(null, [1, 2, 3]); // NaN or Type 'number' is not assignable to type '{ a: ... }'

The compiler will emit the errors above only if strictBindCallApply is on.

Overloads

A function which has overloads is tricky to refactor. Changing a function overload or a function implementation which has overloads can be a breaking change. The overloads and the implementation should be considered together when refactoring. It is not clear what the refactor should do with overloads, so the refactor will not be available in that case.

Example:

function foo(a: number, b: number): number;
function foo(a: string, b: number): string;
function foo(a: string | number, b: number): string | number {
    // implementation
    return 0;
}

might become

function foo(a: number, b: number): number; // Overload signature is not compatible with function implementation
function foo(a: string, b: number): string;
function foo({ a, b }: { a: string | number, b: number }): string | number {
    // implementation
    return 0;
}

or

function foo(a: number, b: number): number;
function foo({ a, b }: { a: string, b: number }): string; // Overload signature is not compatible with function implementation
function foo(a: string | number, b: number): string | number {
    // implementation
    return 0;
}

Overrides

As with overloads, refactoring overrided methods can also be a breaking change. Refactoring a method which overrides another can mean that the refactored method's type is no longer assignable to the type of the method it overrides. Similarly, changing a method which is overriden by another can mean that the overriding methods' types are no longer assignable to the refactored method.

Example:

class A { 
    foo(a: string, b: string) { }
}
class B extends A { 
    foo(c: string, d: string) { }
}

would become

class A { 
    foo({ a, b }: { a: string, b: string }) { }
}
class B extends A { 
    foo(c: string, d: string) { } // error: property 'foo' in type B is not assignable to the same property in base type 'A'
}

That scenario is currently avoided by the refactor. Once it checks all the references and finds a declaration different than the one where the refactor should be applied, it does not perform any changes.

Inference

When a function is refactored to have an object parameter instead of a list of parameters, inference can no longer proceed left-to-right on the parameter list, so the compiler no longer reuses inferences from previous parameters for the next parameter.
Because of that, inference can become worse and cause breaks in rare cases (see #29791).

Known issues

Formatting

In some cases involving comments and line breaks, the refactor generates an argument object that is ill-formatted.
The refactorConverToNamedParameters_callComments2.ts test is currently wrong because of that.

Inherited constructors

Currently findAllReferences will not track usage of an inherited constructor.

Example:

class Foo {
    constructor(t: string, s: string) { }
}
class Bar extends Foo { }
var bar = new Bar("a", "b");

refactoring Foo's constructor, becomes:

class Foo {
    constructor({ t, s }: { t: string; s: string; }) { }
}
class Bar extends Foo { }
var bar = new Bar("a", "b"); // Error: Expected 1 arguments, but got 2.

The refactorConvertToNamedParameters_inheritedConstructor.ts test is currently wrong because of that.
However, findAllReferences tracks inherited methods, which causes the refactor to behave differently in similar situations when it should behave the same. This leads me to believe that findAllReferences should be able to find references to inherited constructors.
Update: solved in #30514.

Type errors when refactoring a method

When a class/object literal's method is refactored, its type changes.
Because of that, we can get type errors where the new class/object literal type is not compatible with the expected type.
Example:

class Bad {
    fn(a: number, b: number) {
        return a + b;
    }
}

interface Expected {
    fn(a: number, b: number): number;
}

declare var e: Expected;
declare var b: Bad;

e = b;
b = e;

After refactoring fn on Bad:

class Bad {
    fn({ a, b }: { a: number; b: number; }) {
        return a + b;
    }
}

interface Expected {
    fn(a: number, b: number): number;
}

declare var e: Expected;
declare var b: Bad;

e = b; // Error: Type 'Bad' is not assignable to type 'Expected'.
b = e; // Error: Type 'Expected' is not assignable to type 'Bad'.

Open questions

Type Literal or Interface

Should the refactor generate a type literal for the new parameter object or an interface?
In the case where this refactor generates a type literal,
this refactor should be composed with a refactor for extracting a type literal into an interface.

Example:

function add(a: number, b: number, c: number): number {
    return a + b + c;
}

could be refactored to

function add({ a, b, c }: { a: number, b: number, c: number }): number {
    return a + b + c;
}

or

interface AddOptions {
    a: number;
    b: number;
    c: number;
}

function add({ a, b, c }: AddOptions) {
    return a + b + c;
}

Name

What should the refactor be called?
Currently it is 'named parameters', but 'parameter object' was suggested.
Update: done in #30401.

Dealing with bad cases

The refactor's current approach is to not apply changes when it finds a reference it does not recognize.
This approach comes from the idea that a refactor should not break user's code, i.e. it should 'do the right thing'.
So, when the refactor does not know the right thing to do, it does nothing.
Some cons:

This approach is very restrictive and will cause the refactor to not do anything in many cases, possibly to the user's frustration.
It requires that we recognize what the "good usages" are and add code for each one.
The question is: is this the best approach?
In some situations, the refactor could break the user's code, and the breaking changes can be detected by the type checker or not.
But if the user then has type errors that indicate all the parts of the code that have to be fixed,
could it be better to apply the refactor and have the user do the remaining necessary changes?

Good cases

For the current approach, what are other usages of the refactored function that the refactor should recognize?

JavaScript

If the JavaScript code is not type annotated, what should we do?

…pyComment function

amcasey · 2019-02-26T00:38:30Z

This is definitely optional, but it would be very nice if we simplified x: x to x. I think passing variables with the same name as parameters is a fairly common pattern.

yortus · 2019-02-26T01:14:34Z

Another 'bad case' to note: #29791. For generic functions, tsc currently currently makes assumptions about parameter order to infer the parameter types. With a named params object this ordering is lost, so type inference may fail on the refactored version.

amcasey

Questions about textChanges

src/services/textChanges.ts

amcasey · 2019-02-26T02:07:25Z

src/services/textChanges.ts

@@ -178,11 +194,12 @@ namespace ts.textChanges {

    function getAdjustedEndPosition(sourceFile: SourceFile, node: Node, options: ConfigurableEnd) {
        const { end } = node;
-        if (options.useNonAdjustedEndPosition || isExpression(node)) {
+        const { endPosition } = options;
+        if (endPosition === TrailingTriviaOption.Exclude || (isExpression(node) && endPosition !== TrailingTriviaOption.Include)) {


This is the only place I can see TrailingTriviaOption.IncludeIfLineBreak being consumed and I don't understand how it relates to expressions.

I don't know why it is necessary to check if node is an expression, but for the behavior I needed for this function (when the TrailingTriviaOption.Include is used), it should not matter if node is an expression. So I just tried not to break existing code. The same goes for the isLineBreak check below, I don't know what is the reasoning behind it.

I think you got rid of IncludeIfLineBreak.

…ingTriviaOption

amcasey

Comments about utilities.ts.

src/services/utilities.ts

amcasey

Initial comments on convertToNamedParameters.

src/services/refactors/convertToNamedParameters.ts

amcasey · 2019-02-26T22:26:16Z

src/services/refactors/convertToNamedParameters.ts

+        }
+        return false;
+
+        function isValidParameterNodeArray(parameters: NodeArray<ParameterDeclaration>): parameters is ValidParameterNodeArray {


How did you decide which helper functions would be nested and which would be siblings?

The ones that don't close over anything should be moved outside to avoid excess closure allocations.

src/services/refactors/convertToNamedParameters.ts

amcasey · 2019-02-26T22:52:08Z

src/services/refactors/convertToNamedParameters.ts

+        const references = flatMap(names, name => FindAllReferences.getReferenceEntriesForNode(-1, name, program, program.getSourceFiles(), cancellationToken));
+        let groupedReferences = groupReferences(references);
+
+        // if the refactored function is a constructor, we must also go through the references to its class


Didn't getDeclarationNames return the class name? I would have thought it was already incorporated into references.

I separated getDeclarationNames into getClassNames and getFunctionNames, and now there's a single pass through the references which checks for both function and (if necessary) class references, so the code flow should be more clear now.

src/services/refactors/convertToNamedParameters.ts

sandersn

Looks good so far. I've only reviewed convertToNamedParameters so far.

src/services/organizeImports.ts

src/services/refactors/convertToNamedParameters.ts

sandersn · 2019-02-27T17:24:42Z

src/services/refactors/convertToNamedParameters.ts

+                /*questionToken*/ undefined,
+                thisParameter.type);
+
+            suppressLeadingAndTrailingTrivia(newThisParameter.name);


Can you explain why we need to suppress trivia and then copy comments instead of just using newThisParameter.name as-is?

If we don't suppress trivia, trailing comments will be emitted but leading comments will disappear. In essence, I do not know what behavior to expect from trivia when reusing nodes. So I am suppressing both leading and trailing and explicitly copying the comments.

amcasey

I believe you're still polishing but it generally LGTM.

amcasey · 2019-02-28T03:02:29Z

src/services/refactors/convertToNamedParameters.ts

+                const classExpression = constructorDeclaration.parent;
+                const variableDeclaration = constructorDeclaration.parent.parent;
+                const className = classExpression.name;
+                if (className) return [className, variableDeclaration.name];


Why do we need the class name if there's a variable name? Is that for ctor calls within the class itself?

yes, that's for checking the references to the class within the class expression itself.

src/services/refactors/convertToNamedParameters.ts

amcasey · 2019-03-01T02:12:44Z

src/services/refactors/convertToNamedParameters.ts

+    function getGroupedReferences(functionDeclaration: ValidFunctionDeclaration, program: Program, cancellationToken: CancellationToken): GroupedReferences {
+        const functionNames = getFunctionNames(functionDeclaration);
+        const classNames = isConstructorDeclaration(functionDeclaration) ? getClassNames(functionDeclaration) : [];
+        const names = deduplicate([...functionNames, ...classNames], equateValues);


I'm not sure I understand why there would be duplicates.

when refactoring the constructor of a class expression, the variable name to which the class is assigned is both a function name and a class name.

sandersn · 2019-03-04T21:37:02Z

tests/cases/fourslash/refactorConvertToNamedParameters_inheritedConstructor.ts

+////var foo = new Foo("c", "d");
+
+goTo.select("a", "b");
+/*  The expected new content is currently wrong.


is this tracked by a bug? It's a good idea to file one after merging this PR.

RyanCavanaugh

Looks great overall. Polling the issue on naming so hold off on changing that, but please do move the functions out one level where appropriate.

RyanCavanaugh · 2019-03-05T23:02:52Z

src/compiler/diagnosticMessages.json

@@ -4871,5 +4871,9 @@
    "Enable the 'experimentalDecorators' option in your configuration file": {
        "category": "Message",
        "code": 95074
+    },
+    "Convert to named parameters": {


I think we agreed on "Convert to parameters object" ?

RyanCavanaugh · 2019-03-05T23:11:37Z

src/services/refactors/convertToNamedParameters.ts

+        }
+        return false;
+
+        function isValidParameterNodeArray(parameters: NodeArray<ParameterDeclaration>): parameters is ValidParameterNodeArray {


The ones that don't close over anything should be moved outside to avoid excess closure allocations.

After spending almost two days in finding the issue, I ran across a few TypeScript issues on their GitHub page: - Loss of type inference converting to named parameters object microsoft/TypeScript#29791 - Parameter of a callback without a specified type next to it breaks code. microsoft/TypeScript#29799 - Convert to named parameters microsoft/TypeScript#30089 It became clear that TypeScript is unable to infer method return arguments if a generic type is used more than once in generic parameter object. Instead it returns {}. For example, the following would fail on line 28: type Convert<A, B> = (value: A) => B interface IParams<C, D> { value: C convert: Convert<C, D> doConvert: (value: C, convert: this['convert']) => D } function doSomething<E, F>(value: E, convert: Convert<E, F>) { return convert(value) } function build<G, H>(params: IParams<G, H>) { const {value, convert} = params return params.doConvert(value, convert) } const outerResult = build({ value: { a: { value: 1, }, b: 'string', }, convert: value => value.a, doConvert: (value, convert) => { const innerResult = doSomething(value, convert) innerResult.value console.log('innerResult:', innerResult) return innerResult }, }) console.log('outerResult:', outerResult) With the message: Property 'value' does not exist on type '{}'. If we replace parameter object IParams with regular ordered function parameters, the compilation succeeds. RyanCavanough (TS project lead) from GitHub commented: > We don't have a separate pass to say "Go dive into the function and > check to see if all its return statements don't rely on its parameter > type" - doing so would be expensive in light of the fact that extremely > few real-world functions actually behave like that in practice. Source: microsoft/TypeScript#29799 (comment) These modifications bring type safety to TestUtils.tsx, and therefore client-side tests of React components, while keeping almost the same ease of use as before.

Gabriela Britto added 30 commits January 15, 2019 16:56

create refactoring for converting to named parameters

9aeeae5

WIP

705ac60

implement new parameter creation

23fc65a

implement new parameter creation

919ed79

add test for convert to named parameters refactoring

bd290e6

Move function getTypeNodeIfAccessible from inferFromUsage to utilities

f3e60be

Refactor direct function calls

3243b4b

Add tests for convert to named parameters refactor

b668e34

Merge branch 'master' into convert-to-named-parameters

9e62eb9

preserve this parameter when refactoring

1d94322

update tests formatting

050c70a

add tests

40987ec

Don't refactor if variable declaration has type annotation

bf25ba4

fix refactor to find super references

bde97d1

add more tests

abb1155

implement refactor for functions with a rest parameter

674800f

add tests for rest parameters

1d1c820

add option to suppressLeadingAndTrailingTrivia non recursively

18b2b55

fix duplication of leading and trailing comments on refactored function

b87392c

Merge branch 'master' into convert-to-named-parameters

9270e58

fix hasThisParameter to check for parameters length

7d86fda

create functions for copying trailing comments and rename previous co…

26f8ddd

…pyComment function

copy comments when refactoring

dba631d

add tests for comments

4e135f1

don't apply refactor when parameter has decorators

dbd8499

don't apply changes when unexpected reference is found

ef4db31

return function expression name in getFunctionDeclarationNames

ec0e734

fix reference checking

05e9d6c

rename refactor tests

b93afff

create new ConfigurableStart and ConfigurableEnd options and rename them

eef3da5

amcasey reviewed Feb 26, 2019

View reviewed changes

Gabriela Araujo Britto added 3 commits February 26, 2019 09:43

change startPosition and endPosition to leadingTriviaOption and trail…

e3a465f

…ingTriviaOption

don't export useNonAjustedPositions

bf5123d

remove unused LeadingTriviaOption and TrailingTriviaOption options

970ec62

amcasey reviewed Feb 26, 2019

View reviewed changes

src/services/utilities.ts Outdated Show resolved Hide resolved

src/services/utilities.ts Show resolved Hide resolved

amcasey reviewed Feb 26, 2019

View reviewed changes

refactor utilities

006fe14

gabritto mentioned this pull request Feb 27, 2019

Improve 'convert to named parameters' refactor #30113

Closed

sandersn requested changes Feb 27, 2019

View reviewed changes

Gabriela Araujo Britto added 5 commits February 27, 2019 16:19

refactor getGroupReferences

54c7996

minor refactors to convertToNamedParameters

7fd6868

use sortAndDeduplicate instead of deduplicate

51616a4

add diagnostics message for refactor description

617d5af

remove inlineable

1f91aa1

amcasey approved these changes Mar 1, 2019

View reviewed changes

Gabriela Araujo Britto added 3 commits March 1, 2019 09:17

use interface for ValidVariableDeclaration

cd060e7

use getTouchingToken

98a146d

add test for non-transient symbol

8c4ab69

sandersn reviewed Mar 4, 2019

View reviewed changes

sandersn approved these changes Mar 4, 2019

View reviewed changes

RyanCavanaugh requested changes Mar 5, 2019

View reviewed changes

remove unnecessary closures

60b2d6a

gabritto merged commit d2364f5 into master Mar 6, 2019

gabritto deleted the convert-to-named-parameters branch March 14, 2019 21:40

DanielRosenwasser mentioned this pull request Apr 5, 2019

Convert to destructed refactoring not enabled in JavaScript? #30764

Open

OliverJAsh mentioned this pull request Dec 1, 2020

"Convert parameters to destructured object": enable for functions with just one parameter #41753

Closed

5 tasks

iisaduan mentioned this pull request Jul 4, 2024

allow more cases to not block convert params to destructured object refactor #59140

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to named parameters #30089

Convert to named parameters #30089

gabritto commented Feb 25, 2019 •

edited

Loading

amcasey commented Feb 26, 2019

yortus commented Feb 26, 2019

amcasey left a comment

amcasey Feb 26, 2019

gabritto Feb 26, 2019

amcasey Feb 28, 2019

amcasey left a comment

amcasey left a comment

amcasey Feb 26, 2019

RyanCavanaugh Mar 5, 2019

amcasey Feb 26, 2019

gabritto Feb 28, 2019

sandersn left a comment

sandersn Feb 27, 2019

gabritto Feb 28, 2019

amcasey left a comment

amcasey Feb 28, 2019

gabritto Mar 1, 2019

amcasey Mar 1, 2019

gabritto Mar 1, 2019

sandersn Mar 4, 2019

RyanCavanaugh left a comment

RyanCavanaugh Mar 5, 2019

RyanCavanaugh Mar 5, 2019

Convert to named parameters #30089

Convert to named parameters #30089

Conversation

gabritto commented Feb 25, 2019 • edited Loading

Convert to "named parameters" refactor

Refactor availability

References

Good cases

Direct calls

Imports & Exports

Other good cases

Good class cases

JSDoc

Rest parameters

This parameter

Parameters with initializers

Optional parameters

JavaScript

Some bad cases

Aliasing

Types

Nameless declarations (anonymous functions/classes)

bind/call/apply calls

Overloads

Overrides

Inference

Known issues

Formatting

Inherited constructors

Type errors when refactoring a method

Open questions

Type Literal or Interface

Name

Dealing with bad cases

Good cases

JavaScript

amcasey commented Feb 26, 2019

yortus commented Feb 26, 2019

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanCavanaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabritto commented Feb 25, 2019 •

edited

Loading

`bind`/`call`/`apply` calls