TypeScript should infer `newLocal` name from property name when performing "Extract to constant in enclosing scope" on a PropertyAccessExpression

[dtinth]

Search Terms

newLocal, Extract to constant in enclosing scope

Suggestion

When performing “Extract to constant in enclosing scope”, if the expression being extracted is a PropertyAccessExpression, I think it would be great if TypeScript used the expression’s name instead of "newLocal", if the name would not clash with other symbols in the scope or keywords.

My suggestion summarized as a patch:

diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts
index 30dcaeaebf..c3b57c61b0 100644
--- a/src/services/refactors/extractSymbol.ts
+++ b/src/services/refactors/extractSymbol.ts
@@ -1009,7 +1009,9 @@ namespace ts.refactor.extractSymbol {
 
         // Make a unique name for the extracted variable
         const file = scope.getSourceFile();
-        const localNameText = getUniqueName(isClassLike(scope) ? "newProperty" : "newLocal", file);
+        const localNameText = isPropertyAccessExpression(node) && !isClassLike(scope) && !checker.resolveName(node.name.text, node, SymbolFlags.Value, false) && !isPrivateIdentifier(node.name) && !isKeyword(node.name.originalKeywordKind!)
+            ? node.name.text
+            : getUniqueName(isClassLike(scope) ? "newProperty" : "newLocal", file);
         const isJS = isInJSFile(scope);
 
         let variableType = isJS || !checker.isContextSensitive(node)

If this sounds good, I’m happy to come up with a proper PR and submit it. Would love to try contributing :).

Use Cases

This would save me a lot of time when I extract stuff to constant (I do this a lot). Without this feature I would have to:

• Copy the name before performing the refactoring, so that I could paste it into the rename box.

  

• Sometimes I forgot to copy before extracting. I had to type the name manually.

• That name may be long and the rename box doesn’t provide auto-completions… I had to dismiss the rename box, copy the name, and come back to the rename box.

  

Examples

Text version

function get(options: { url: string }) {
  return fetch(options.url).then(r => r.json())
  //           ^^^^^^^^^^^
  //           Extract to constant in enclosing scope 
}

Current behavior: The variable name is always newLocal.

function get(options: { url: string }) {
  const newLocal = options.url;
  return fetch(newLocal).then(r => r.json())
}

Suggested behavior: The variable name is url.

function get(options: { url: string }) {
  const url = options.url;
  return fetch(url).then(r => r.json())
}

Checklist

My suggestion meets these guidelines:

• This wouldn't be a breaking change in existing TypeScript/JavaScript code
• This wouldn't change the runtime behavior of existing JavaScript code
• This could be implemented without emitting different JS based on the types of the expressions
• This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
• This feature would agree with the rest of TypeScript's Design Goals.

[dtinth]

I created a branch with the changes in place, as well as test for each added conditional logic. If this looks satisfactory I can open a PR.

master...dtinth:extract-constant-property-name

I intentionally made the scope for the PR narrow and incremental to be conservative. Also a lot of cases are excluded, I believe this incremental change could be useful. 😄

Rationale for each exclusion

• isPropertyAccessExpression(node) — While there are more potentials to infer from other things, like from method/function’s name (getSomething → something) or from its type (await fetch(url) → response), the naming heuristics aren’t as straightforward as that of a property access.

• !isClassLike(scope) — When a class property already exists, without this guard, it generated a duplicate name. So I excluded it. There is already a test covering this case.

• !checker.resolveName(node.name.text, node, SymbolFlags.Value, false) — Don't use a name if it’d conflict or shadow as names in outer scopes.

• !isPrivateIdentifier(node.name) — PrivateIdentifiers don’t have originalKeywordKind to be used with the next clause, so rather than creating a brand new Identifier node and check its originalKeywordKind, I excluded it instead.

• !isKeyword(node.name.originalKeywordKind!) — prevents clashing with reserved words, like x.if.

[dtinth]

I’ve opened the PR, since the conditions for a PR has been met. (Issue went in the Backlog milestone; “help wanted” label added)

[ilogico]

Since this is already a particular case, I would prefer it would refactor to const { url } = options; instead.

[dtinth]

@RyanCavanaugh Thanks for merging the PR #37902 — it’s working on my VS Code with JavaScript and TypeScript Nightly extension now.

Should this issue be put on the TypeScript 4.7.0 milestone?