allow more cases to not block convert params to destructured object refactor
#59140
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
iisaduan
changed the title
convert params to destructured object refactor
There was a problem hiding this comment.
Left minor comments, but in general I think this looks ok to me. One thing I'm wondering, though, is that if we are willing to allow the refactor to be applied even when we know it will introduce errors, we shouldn't simply allow all cases (i.e. more than the ones in this PR), and rely on the fact that there will be type errors to alert the user to the places where the code needs to be manually updated.
| if (entry.node.parent) { | ||
| const functionReference = entry.node; | ||
| const parent = functionReference.parent; | ||
| let callOrNewExpression: CallExpression | NewExpression | undefined; | ||
| let matchReference = functionReference; |
There was a problem hiding this comment.
nit: could we assign functionReference to matchReference inside the first case of the switch, to match the other two cases?
| } | ||
|
|
||
| if (callOrNewExpression === undefined) return undefined; | ||
| if (callOrNewExpression.expression === matchReference) return callOrNewExpression; |
There was a problem hiding this comment.
Could you move the comments on the switch cases to here? Something saying that if callOrNewExpression.expression === matchReference, then it means we have one of the cases of e.g. x.foo(...) or x["foo"](...) or foo(...) or super(...) or new Foo(...).
|
|
||
| if (callOrNewExpression === undefined) return undefined; | ||
| if (callOrNewExpression.expression === matchReference) return callOrNewExpression; | ||
| if (isCallExpression(callOrNewExpression) && callOrNewExpression.arguments.some(arg => arg === matchReference)) return true; |
There was a problem hiding this comment.
Similar to my comment above, could you add comments here saying that if this is true, we have a reference like map(x, foo) or x.map(foo), etc.?
| @@ -355,47 +366,63 @@ function entryToImportOrExport(entry: FindAllReferences.NodeEntry): Node | undef | |||
| return undefined; | |||
| } | |||
|
|
|||
| function entryToDeclaration(entry: FindAllReferences.NodeEntry): Node | undefined { | |||
| function entryToDeclaration(entry: FindAllReferences.NodeEntry): Node | true | undefined { | |||
| // returns a callOrNewExpression if the reference needs to be updated during the refactor | |||
There was a problem hiding this comment.
I think technically this returns a Node whose parent is a declaration.
| function entryToDeclaration(entry: FindAllReferences.NodeEntry): Node | true | undefined { | ||
| // returns a callOrNewExpression if the reference needs to be updated during the refactor | ||
| // returns 'true' if the reference will not be updated by the refactor, but should not block the refactor | ||
| // returns 'undefined' if the reference should block the refactor | ||
| if (isDeclaration(entry.node.parent)) { | ||
| return entry.node; |
There was a problem hiding this comment.
I think this also needs to be modified to return true in some cases. For instance, the following test case that is similar to the method assignment one, results in the refactor failing to be applied:
function /*a*/foo/*b*/(actual: number) {
return actual;
}
const x = foo;That's because const x = foo is a declaration, so that foo reference will be added to the grouped reference's declarations, but it will later be considered invalid here.
There was a problem hiding this comment.
I don't know what I did, but I thought I had tested that case out and it wasn't blocked! I'll add it.
I was thinking the same (hence why the draft)-- the original bug report provided a case where there was just one broken reference in a test case, but I can see how it could get bad fast if there are a lot of broken cases. And if we allow this refactor to break this case, why not others? |
fixes #58930
^ was purposefully disabled in #30089 (comment):
TODO: update description more.