Fixes rename for destructuring, named imports and default imports #7945
Conversation
sheetalkamat
changed the title
| @@ -5624,8 +5624,34 @@ namespace ts { | |||
| }; | |||
| } | |||
|
|
|||
| function isImportSpecifierSymbol(symbol: Symbol) { | |||
| return (symbol.flags & SymbolFlags.Alias) && !!getDeclarationOfKind(symbol, SyntaxKind.ImportSpecifier); | |||
| function getImportOrExportSpecifierPropertyNameSymbolSpecifier(symbol: Symbol, location: Node): ImportOrExportSpecifier { | |||
There was a problem hiding this comment.
getImportOrExportSpecifierForPropertyNameSymbol?
|
Is there a way to open the scope of this for object assignment patterns? interface I {
x: number;
}
var a: I;
var x;
({ /*renameHere*/x: x } = a); |
|
That eg. gets handled by ad916ab. I added the test case for it just now. |
| function checkArrayLiteralDestructuringElementAssignment(node: ArrayLiteralExpression, sourceType: Type, | ||
| element: Expression, index: number, elementType: Type, contextualMapper?: TypeMapper) { | ||
| const elements = node.elements; | ||
| if (element.kind !== SyntaxKind.OmittedExpression) { |
There was a problem hiding this comment.
Does this implicitly return undefined? Invert the conditions here to reduce nesting and make the returns explicit.
|
|
||
| // If the reference location is in an object literal, try to get the contextual type for the | ||
| // object literal, lookup the property symbol in the contextual type, and use this symbol to | ||
| // compare to our searchSymbol | ||
| if (isNameOfPropertyAssignment(referenceLocation)) { | ||
| return forEach(getPropertySymbolsFromContextualType(referenceLocation), contextualSymbol => { | ||
| const contexualSymbol = forEach(getPropertySymbolsFromContextualType(referenceLocation), contextualSymbol => { |
There was a problem hiding this comment.
ahh! thanks for catching this
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| ////interface I { | ||
| //// property1: number; |
There was a problem hiding this comment.
So why exactly wouldn't property1 cascade to this declaration?
There was a problem hiding this comment.
Becauyse you are finding ref on var property1 which is different from property property1
There was a problem hiding this comment.
We should fix that at some point, but not here
There was a problem hiding this comment.
Don't think you want to combine that.. Just because you happen to rename the name of variable (say so it contains all types of properties at various instances) doesn't mean you want to rename the properties. So current behavior is what you want there
There was a problem hiding this comment.
Potentially, but then you end up breaking code in at least one other place.
There was a problem hiding this comment.
Which would need intentional rename which is good. that is either user has to intentionally rename other variuable or use as clause
…to getPropertySymbolOfObjectBindingPatternWithoutPropertyName
| @@ -23,6 +23,7 @@ let ranges = test.ranges(); | |||
| for (let range of ranges) { | |||
| goTo.position(range.start); | |||
|
|
|||
| debugger; | |||
| //// export {a as somethingElse} | ||
| //// We want the *local* declaration of 'a' as declared in the import, | ||
| //// *not* as declared within "mod" (or farther) | ||
| const importOrExportSpecifier = getImportOrExportSpecifierForPropertyNameSymbol(symbol, location); |
There was a problem hiding this comment.
i would push this logic in getImportOrExportSpecifierForPropertyNameSymbol instead of duplicating it twice
|
👍 |
Fixes #6312, #7708 and #7024