fixes(44110): allow "extract to inner function" refactoring inside function expression #44206
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
|
| @@ -17,4 +17,4 @@ | |||
|
|
|||
| goTo.select("start", "end"); | |||
| verify.refactorAvailable("Extract Symbol", "function_scope_1"); | |||
| verify.not.refactorAvailable("Extract Symbol", "function_scope_2"); | |||
| verify.refactorAvailable("Extract Symbol", "function_scope_2"); | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| functionErrorsPerScope.push( | ||
| isFunctionLikeDeclaration(scope) && scope.kind !== SyntaxKind.FunctionDeclaration | ||
| ? [createDiagnosticForNode(scope, Messages.cannotExtractToOtherFunctionLike)] | ||
| : []); |
There was a problem hiding this comment.
I'm sure there was a good reason not to allow extractions for functions which aren't declarations…
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
OliverJAsh
changed the title
typescript-bot
added
For Backlog Bug
|
I can think of a couple reasons arrow functions might have been avoided. Why no extract to arrow function? It would double the number of available refactors at a given extraction location. It would also have to be extracted to the right location to avoid runtime errors, but since we would know that it would only be called once (at the new location) the refactor on its own seems fine. Why no convert to arrow function? The original declaration would have to show up before all of it's call sites or the refactor would introduce a runtime error, and I think the preference in general is to push declarations to the bottom of a scope. It might be doable, but I don't think it would be obvious to people why the refactor would only show up on function declarations some of the time if we restricted the cases. These are guesses on my part and may be bad reasons. Either way, I can't think of why we can't offer a function declaration inside of an expression. |
Do you think we could move forward with this PR then, or do you want to wait for more feedback first? |
|
yes, just looking over the tests :) |
|
To be clear this PR also allows extract to inner function to work for class methods and constructors as well. Is there a case for wanting to extract an inner function in a constructor? This seems like it would be a weird choice to me. |
I wouldn't like to make assumptions about where these tools would be useful. If you have code in a scope anywhere, you might want to extract it—whether that's to an inner function or a module level function really depends on the author's use case and style preferences. Maybe they want to extract it to an inner function so it's easier to move later on, if it's needed elsewhere. |
|
Fair point. It should be easy enough to restrict those cases if we decide later that offering them is too much clutter. |
Fixes #44110