Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'move to new file' refactor #23726

Merged
15 commits merged into from
May 10, 2018
Merged

Add 'move to new file' refactor #23726

15 commits merged into from
May 10, 2018

Conversation

ghost
Copy link

@ghost ghost commented Apr 26, 2018

The user may highlight declarations on the top-level of a file and move them to a new file. Imports will be updated, and declarations will be exported as necessary (if they were private in the old file but used in the new file, or vice-versa).

This doesn't work for nested declarations because those may need to close over things -- extractSymbol handles that and adds parameters as necessary. So it would take two refactorings to take something from an inner scope to a different file.

@@ -4174,5 +4174,9 @@
"Generate 'get' and 'set' accessors": {
"category": "Message",
"code": 95046
},
"Move to new file": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to a new file ?

@@ -681,6 +694,11 @@ namespace ts.textChanges {
});
}

export function newFileChanges(oldFile: SourceFile, fileName: string, statements: ReadonlyArray<Statement>, newLineCharacter: string): FileTextChanges {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check with @mjbvz and @amcasey that sending edits for non-existing files are ok on the editor sides.

const range = createTextRangeFromSpan(getRefactorContextSpan(context));
const { statements } = file;

const startNodeIndex = findIndex(statements, s => s.end > range.pos);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so that means that if i select a nested function to move, we will move the containing function instead.. would not it be better to not make the refactoring available at this case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See moveToNewFile_rangeInvalid.ts.

): ReadonlyArray<Statement> {
const checker = program.getTypeChecker();

if (!oldFile.externalModuleIndicator) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if this is a .js file with require calls?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also what should we do with require statements in a .js file. seems like we have enough information in the current file to know if we should create import or const x = require(...)..

});
}

function deleteUnusedImports(sourceFile: SourceFile, importDecl: ImportDeclaration, changes: textChanges.ChangeTracker, isUnused: (name: Identifier) => boolean): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks a lot like the logic we use in fixUnusedIdentifer for module imports.. can we consolidate?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's easy since fixUnusedIdentifier deletes a single identifier starting from the identifier, but here we have a set of symbols and want to recurse down starting from the sourcefile and delete everything in the set.


function deleteUnusedOldImports(oldFile: SourceFile, toMove: ReadonlyArray<Statement>, changes: textChanges.ChangeTracker, toDelete: ReadonlySymbolSet, checker: TypeChecker) {
for (const statement of oldFile.statements) {
if (!contains(toMove, statement) && isImportDeclaration(statement)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about ImportEqualsDeclaration?

while (true) {
const name = combinePaths(inDirectory, moduleName + extension);
if (!host.fileExists(name)) return moduleName;
moduleName += "0";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, i would make that 1-based (more natural) and i would use a separator of sorts. VSCode adds a .1, Windows will add (2) to duplicate files.
we could go with the .1 or we can use _1.


function getNewModuleName(movedSymbols: ReadonlySymbolSet): string {
let name: string | undefined;
movedSymbols.forEach(s => { if (name === undefined) name = symbolNameNoDefault(s); });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return forEach(movedSymbols, symbolNameNoDefault) || "newFile";

});

const oldFileImport = makeImportIfNecessary(oldFileDefault, oldFileNamedImports, `./${removeFileExtension(getBaseFileName(oldFile.fileName))}`);
return [...copiedOldImports, ...(oldFileImport ? [oldFileImport] : emptyArray)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, why not just push on copiedOldImports instead of cloning it.

if (isInImport(decl)) {
oldImportsNeededByNewFile.add(symbol);
}
else if (isTopLevelDeclaration(decl) && !movedSymbols.has(symbol)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if it is not in movedSymbols just case we have not visited it yet, e.g. a forward reference to a type or an interface.. or even a class, that exists later on in the selected range?

function addEs6Export(d: TopLevelDeclarationStatement): TopLevelDeclarationStatement {
const modifiers = concatenate([createModifier(SyntaxKind.ExportKeyword)], d.modifiers);
switch (d.kind) {
case SyntaxKind.FunctionDeclaration:
Copy link
Contributor

@mhegazy mhegazy May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also export import A = N

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check with @amcasey and @mjbvz regarding new file changes.

@markusjohnsson
Copy link
Contributor

Fixes #13859

@ghost
Copy link
Author

ghost commented May 2, 2018

New commit fixes #23793

node.expression = parenthesizeExpressionForExpressionStatement(expression);
return node;
return createExpressionStatement(parenthesizeExpressionForExpressionStatement(expression));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: extra newline.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider linting for no-padding.

@mhegazy
Copy link
Contributor

mhegazy commented May 2, 2018

@andy-ms we need to make this opt-in to avoid old versions of VS/VSCode form getting edits including new files.
so we need a new setting, allowNewFileEdits or something along these lines.

@mhegazy
Copy link
Contributor

mhegazy commented May 2, 2018

we also need to add the file to "files" in tsconfig.json if the original file was in the list.

@mhegazy mhegazy mentioned this pull request May 3, 2018
@ghost
Copy link
Author

ghost commented May 4, 2018

Latest commit adds an allowTextChangesInNewFiles preference since older editors might have problems with the new behavior. CC @mjbvz

@@ -311,8 +311,8 @@ namespace ts {
}

/** Works like Array.prototype.findIndex, returning `-1` if no element satisfying the predicate is found. */
export function findIndex<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean): number {
for (let i = 0; i < array.length; i++) {
export function findIndex<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean, startIndex = 0): number {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you instead make it startIndex?: number and use let i = startIndex || 0 ?

// If previous file was global, this is easy.
changes.createNewFile(oldFile, combinePaths(currentDirectory, newFileNameWithExtension), getNewStatements(oldFile, usage, changes, toMove, program, newModuleName));

addNewFileToTsconfig(program, changes, normalizePath(combinePaths(oldFile.fileName, "..", newFileNameWithExtension)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not that be the filename relative to the tsconfig.json path?

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial impressions

const refactorName = "Move to a new file";
registerRefactor(refactorName, {
getAvailableActions(context): ApplicableRefactorInfo[] {
if (getStatementsToMove(context) === undefined || !context.preferences.allowTextChangesInNewFiles) return undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second check looks cheaper. Would it make sense to do that first?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
});

function getStatementsToMove(context: RefactorContext): ReadonlyArray<Statement> | undefined {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could/should this logic be shared with extract function/constant?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The corresponding function there would be getRangeToExtract. That seems to have a lot of extract-symbol-specific logic in it, though. And here we should only be moving top-level statements.

];
}

function deleteUnusedOldImports(oldFile: SourceFile, toMove: ReadonlyArray<Statement>, changes: textChanges.ChangeTracker, toDelete: ReadonlySymbolSet, checker: TypeChecker) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the goal of this function is to remove the imports that were only needed for the code being moved. It seems though, like it might also remove imports that were unused to begin with. Personally, I think it seems strange to do a partial Organize Imports as part of this operation. If we are going to do so, then my preference would be to have the editor trigger (unless the server has some extra knowledge?) so that it can appear separately on the undo stack.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unusedImportsFromOldFile will be a subset of oldImportsNeededByNewFile, so we won't remove purely-unused imports.

/// <reference path='fourslash.ts' />

// @Filename: /a.ts
////import { a, b } from "./other";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I'm sure this is clear from the product code, but I haven't read it in detail.] What happens if you extract an import statement. Is that disallowed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a return statement? Can a return statement be moved to another file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you extract an import statement

I think we should allow this, though there's a bug if the moved import is still needed in the old file. #23968

Can a return statement be moved to another file?

Only top-level statements can be moved, and 'return' at top-level is an errorany way.

@ghost
Copy link
Author

ghost commented May 9, 2018

@amcasey Good to go?

@ghost ghost merged commit 7271ec1 into master May 10, 2018
@ghost ghost deleted the moveToNewFile branch May 10, 2018 18:17
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants