getEditsForFileRename: Support directory rename · Pull Request #24305 · microsoft/TypeScript

ghost · 2018-05-21T21:22:19Z

sheetalkamat · 2018-05-21T22:37:06Z

-                if (!program.getSourceFileFromReference(sourceFile, ref) && resolveTripleslashReference(ref.fileName, sourceFile.fileName) === oldFilePath) {
-                    result.push({ sourceFile, toUpdate: ref });
-                }
+                if (program.getSourceFileFromReference(sourceFile, ref)) continue;


why would you want to skip if ref was resolved to some sourceFile?
if you had
index.ts:

/// <ref path="./file1.ts"/>

file1.ts

// Some content

You can rename file1.ts to file2.ts and want to change ref in index to file2.ts?

You're right, I did a similar thing in #24328 but I'll have to update it in this PR too.

sheetalkamat · 2018-05-21T22:42:10Z

+        const rel = getRelativePathFromFile(oldFileOrDirPath, newFileOrDirPath, createGetCanonicalFileName(hostUsesCaseSensitiveFileNames(host)));
+        return (fullPath, relPath, isImport) => {
+            if (fullPath === oldFileOrDirPath) {
+                const newRelPath = pathIsRelative(relPath)


Nit: Pls rename relPath to say textInFile or something like that since it could be non relatvie?

sheetalkamat · 2018-05-22T20:33:55Z

                    : program.getResolvedModuleWithFailedLookupLocationsFromCache(importStringLiteral.text, sourceFile.fileName);
-                if (resolved && contains(resolved.failedLookupLocations, oldFilePath)) {
-                    result.push({ sourceFile, toUpdate: importStringLiteral });
+                const updated = resolved && firstDefined(resolved.resolvedModule ? [resolved.resolvedModule.resolvedFileName] : resolved.failedLookupLocations, path => pathUpdater(path, importStringLiteral.text, /*isImport*/ true));


I think you want to check failed lookup locations irrespective of if the resolvedFileName is present because those failedLookups take precedence over whatever was resolved ?

If it resolved successfully:

To something not in the old directory: We should leave it alone, right?

To something in the old directory: Then we'll recognize that at resolvedFileName and we're done with no need to continue on to failedLookupLocations, right?

Can you think of a case where this isn't sufficient?

Even if it resolved to something not in oldDirectory, if it failed on something in old directory thats going to change the resolution no?

Ok ignore previous (i got confused between invalidation of resolution and change in name of the resolution), why would you want to change name of import if the changes are happening in file that's present in failed lookup locations of resolution

That's relevant if the rename has already happened and we've already updated the program. In that case there will be lots of unresolved imports that are trying to import from oldFile, but since that doesn't exist their resolution will fail.
So we have to look through their failed lookup locations to see if they could have resolved to oldFile if it still existed.
Note that means that we should only look at failed lookup locations if the import wasn't already successfully resolved.

When I have this structure:
Folders b and c are in a
There is node_modules in b where resolution in file a/b/file1.ts has failed lookup locations from a/b/node_modules/ because of say import {a} from "foo" You dont want to rename "foo" to something else right when renaming a/b to a/d ?

Newest commit should fix this, and #24345

sheetalkamat · 2018-05-24T17:33:51Z

+                // e.g., Importing from "/old/a/b", suffix is "/a/b", and we'll leave that part alone.
+                const relToOld = removeDirectoryPrefixFromPath(oldFileOrDirPath, fullPath);
+                if (relToOld === undefined) return undefined;
+                const suffix = isImport ? pathToImportPath(relToOld) : relToOld;


moving folder /a/b/ to folder say /a/c would need to move "resolvedFile name" of` import "foo" say ```/a/b/node_modules/foo/index.d.ts``` will make suffix as : ```"node_modules/foo"``` ? anmd newPrefix as ```"/a/c"``` and resulting import would be````"/a/c/node_modules/foo"``` ?

This function will only be called for imports from outside the moved directory, which won't resolve to node_modules in the moved directory unless it was an explicit relative import from "./b/node_modules/foo".

ghost · 2018-05-24T18:37:42Z

Note: filed #24384 for handling additional tsconfig settings.

sheetalkamat · 2018-05-24T18:38:26Z

+        return (sourceFile, importText) =>
+            !pathIsRelative(importText) ||
+                importIsInternalToDirectory(oldFileOrDirPath, sourceFile.fileName, importText) ||
+                importIsInternalToDirectory(newFileOrDirPath, sourceFile.fileName, importText)


If import text is relative and it was relative path resolving in newDirectory, you need to change the text right?
"/a/b/" to "/a/c" and my import text was "in/a/b/file1" was referring to "../c/file3" you want to change it to "./file3" ?
Would it be better to use getRelativePathToDirectoryOrUrl with file and reference new locations instead ?

If /a/b is being renamed to /a/c, /a/c must not already exist -- so an import to ../c/file3 would have been an error before, right?

I think I see what you mean in the file case, see latest commit.

mhegazy · 2018-05-24T18:51:02Z

Please port to release-2.9 as well.

sheetalkamat · 2018-05-24T19:20:10Z

+            if (fullPath === oldFileOrDirPath) {
+                const newImportText = pathIsRelative(importText)
+                    ? combinePathsSafe(tryRemoveIndexOrPackage(fullPath) && !endsWithSomeIndex(importText) ? importText : getDirectoryPath(importText), rel)
+                    : newFileOrDirPath;


With amd resolution, renaming /a/folder/oldModule.ts to /a/folder/newModule.ts and if I had file as

// /a/folder/someApp.ts import { a } from "oldModule"

to change to

// /a/folder/someApp.ts import { a } from "/a/folder/newModule"

Filed #24385

sheetalkamat · 2018-05-24T19:29:56Z

+    }
+
+    function importIsInternalToDirectory(oldDir: string, sourceFilePath: string, importText: string): boolean {
+        return !!removeDirectoryPrefixFromPath(oldDir, combineNormal(getDirectoryPath(sourceFilePath), importText));


For all these operations you want to take into account the case sensitivity as well (since you are using fileName instead of path)

ghost · 2018-05-24T22:49:11Z

Latest commit fixes #24385

…h instead of updating relative paths

ghost · 2018-05-29T18:31:25Z

Latest commit fixes #24384

ghost · 2018-05-30T20:44:39Z

Also fixes #24493

sheetalkamat · 2018-05-30T21:04:19Z

-        const configFile = program.getCompilerOptions().configFile;
+    /** If 'path' refers to an old directory, returns path in the new directory. */
+    type PathUpdater = (path: string) => string | undefined;
+    function getPathUpdater(oldFileOrDirPath: string, newFileOrDirPath: string): PathUpdater {


This also needs to consider case-sensitive vs case-insensitive

sheetalkamat · 2018-05-30T21:05:12Z

+            if (!isPropertyAssignment(property) || !isStringLiteral(property.name)) continue;
+            switch (property.name.text) {
+                case "files":
+                case "include":


include could be "src/*.ts" and your old path will be oldFileOrDirPath will be full Path correct? (eg. /user/username/project/src). Don't you want to handle that?

If you are renaming file from "/projects/src/file1.ts" to "/projects/lib/file1.ts" and include is "src/*.ts" do you want to add "lib/file1.ts" in the include section ?

sheetalkamat · 2018-05-30T21:26:51Z

+                case "baseUrl":
+                case "typeRoots":
+                case "mapRoot":
+                case "rootDir":


PathMapping needs to be updated too ?

sheetalkamat · 2018-05-31T20:12:38Z

+            case "typeRoots":
+            case "mapRoot":
+            case "rootDir":
+            case "rootDirs":


except files, includes and exclude rest of these are in "compilerOptions" property. So you will miss those?

sheetalkamat · 2018-05-31T20:15:52Z

+    function getPathUpdater(oldFileOrDirPath: string, newFileOrDirPath: string, getCanonicalFileName: GetCanonicalFileName): PathUpdater {
+        const canonicalOldPath = getCanonicalFileName(oldFileOrDirPath);
+        return path => {
+            const canonicalPath = getCanonicalFileName(path);


This still handles the path as is but you need to be able to handle include/exclude/file like:
"files": "src/foo.ts" or "./src/foo.ts", "../src/foo.ts" etc?

For include/exclude we don't just call this directly, see tryUpdateString. First we prepend the config directory to the path, call oldToNew, then convert to a relative path again.

sheetalkamat · 2018-05-31T21:23:19Z

+                            case "mapRoot":
+                            case "rootDir":
+                            case "rootDirs":
+                                updatePaths(property);


I think you want to use isFilePath from getOptionNameMap.get(propertyName) to update the paths? instead of listing all these here?

sheetalkamat

Except suggestion to use getOptionNameMap().get().isFilePath changes look good.

mhegazy · 2018-05-31T21:53:12Z

@Andy-MS please port to release-2.9 as well.

* getEditsForFileRename: Support directory rename * Code review * Handle imports inside the new file/directory * Document path updaters * Shorten relative paths where possible * Reduce duplicate code * Rewrite, use moduleSpecifiers.ts to get module specifiers from scratch instead of updating relative paths * Update additional tsconfig.json fields * Add test with '.js' extension * Handle case-insensitive paths * Better tsconfig handling * Handle properties inside compilerOptions * Use getOptionFromName

getEditsForFileRename: Support directory rename

d14e981

ghost requested review from amcasey and sheetalkamat May 21, 2018 21:22

sheetalkamat reviewed May 21, 2018

View reviewed changes

Code review

af54bda

sheetalkamat reviewed May 22, 2018

View reviewed changes

Andy Hanson added 2 commits May 22, 2018 15:06

Merge branch 'master' into getEditsForFileRename_directory

c36a12e

Handle imports inside the new file/directory

2f8edbd

ghost force-pushed the getEditsForFileRename_directory branch from 26c5d7b to 2f8edbd Compare May 23, 2018 18:52

sheetalkamat reviewed May 24, 2018

View reviewed changes

Merge branch 'master' into getEditsForFileRename_directory

ad0bcf1

sheetalkamat reviewed May 24, 2018

View reviewed changes

Document path updaters

f9c59b1

Andy Hanson added 2 commits May 24, 2018 12:05

Shorten relative paths where possible

8231728

Reduce duplicate code

49757c6

sheetalkamat reviewed May 24, 2018

View reviewed changes

ghost mentioned this pull request May 24, 2018

getEditsForFileRename: Treat global import as relative given AMD module resolution #24385

Closed

sheetalkamat reviewed May 24, 2018

View reviewed changes

Rewrite, use moduleSpecifiers.ts to get module specifiers from scratc…

27f71fa

…h instead of updating relative paths

ghost force-pushed the getEditsForFileRename_directory branch from 4f727db to 27f71fa Compare May 29, 2018 18:11

Update additional tsconfig.json fields

e4fa6da

Andy Hanson added 2 commits May 29, 2018 11:31

Merge branch 'master' into getEditsForFileRename_directory

09fed05

Merge branch 'master' into getEditsForFileRename_directory

ccdd465

ghost force-pushed the getEditsForFileRename_directory branch from 8b39db7 to ccdd465 Compare May 30, 2018 20:40

Add test with '.js' extension

84ad342

sheetalkamat reviewed May 30, 2018

View reviewed changes

Andy Hanson added 2 commits May 30, 2018 15:12

Handle case-insensitive paths

25503e9

Better tsconfig handling

bc75e67

ghost force-pushed the getEditsForFileRename_directory branch from 249888a to bc75e67 Compare May 31, 2018 19:56

sheetalkamat reviewed May 31, 2018

View reviewed changes

Handle properties inside compilerOptions

a43fae1

sheetalkamat reviewed May 31, 2018

View reviewed changes

sheetalkamat approved these changes May 31, 2018

View reviewed changes

Use getOptionFromName

2a73698

ghost force-pushed the getEditsForFileRename_directory branch from 7457ef9 to 2a73698 Compare June 1, 2018 14:41

ghost merged commit d671c7a into master Jun 1, 2018

ghost deleted the getEditsForFileRename_directory branch June 1, 2018 15:23

ghost mentioned this pull request Jun 1, 2018

getEditsForFileRename: Support directory rename (#24305) #24568

Merged

ghost mentioned this pull request Jun 4, 2018

Import update on file move not working for non-relative file paths #24544

Closed

microsoft locked and limited conversation to collaborators Jul 31, 2018

This pull request was closed.

Conversation

ghost commented May 21, 2018

Uh oh!

sheetalkamat May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheetalkamat May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented May 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 24, 2018

Uh oh!

ghost commented May 29, 2018

Uh oh!

ghost commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost May 31, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheetalkamat left a comment

Choose a reason for hiding this comment

Uh oh!

mhegazy commented May 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

sheetalkamat May 21, 2018 •

edited

Loading

sheetalkamat May 24, 2018 •

edited

Loading

ghost May 31, 2018 •

edited by ghost

Loading