Make GetEditsForFileRenameRequestArgs not extend FileRequestArgs #25052

ghost · 2018-06-18T18:50:56Z

amcasey

Thanks!

amcasey · 2018-06-18T20:37:42Z

src/server/session.ts

+            const oldProject = this.projectService.getDefaultProjectForFile(oldFilePath);
+            if (oldProject) return { file: oldFilePath, project: oldProject };
+
+            const newFilePath = toNormalizedPath(args.newFilePath);


I don't know about the VS Code calling pattern, but VS will be making the request after the rename happens so I would expect the "new" code path to succeed much more often (i.e. might be worthwhile to check it first).

amcasey · 2018-06-18T20:38:30Z

src/server/editorServices.ts

-                if (!scriptInfo) {
-                    return Errors.ThrowNoProject();
+        getDefaultProjectForFile(fileName: NormalizedPath, ensureProject = false): Project | undefined {
+            const scriptInfo = this.getScriptInfoForNormalizedPath(fileName);


I would have guessed we'd want all projects for the path. Is that just out of scope for this PR?

Do not change the public method's default here.

Tracking with #25058

This should return the same result as before if ensureProject is provided, though previously it didn't have a default and was non-optional.

The parameter ensureProject was intentional so that we could ensure the passed in parameter and intent (syntactic/semantic/project update needed/not needed)etc

amcasey · 2018-06-18T20:40:56Z

src/compiler/utilities.ts

@@ -7494,7 +7494,11 @@ namespace ts {
        }
    }

-        // Reserved characters, forces escaping of any non-word (or digit), non-whitespace character.
+    export function startsWithDirectory(path: string, dirPath: string): boolean {
+        return tryRemoveDirectoryPrefix(path, dirPath) !== undefined;


Are the paths already in a form that accounts for platform case-sensitivity?

Working on this in #24975

Also, this PR never uses the function any more.

amcasey · 2018-06-18T20:42:39Z

src/server/editorServices.ts

+            });
+            if (!scriptInfo) return undefined;
+            const file = toNormalizedPath(scriptInfo.path);
+            return { file, project: this.ensureDefaultProjectForFile(file) };


I'm not sure I understand why we're calling ensureDefaultProjectForFile. Is it to trigger a refresh? Otherwise, it seems like scriptInfo.getDefaultProject() would suffice.

…roject()

sheetalkamat · 2018-06-18T21:07:23Z

src/server/session.ts

@@ -1454,7 +1466,7 @@ namespace ts.server {
        private createCheckList(fileNames: string[], defaultProject?: Project): PendingErrorCheck[] {
            return mapDefined<string, PendingErrorCheck>(fileNames, uncheckedFileName => {
                const fileName = toNormalizedPath(uncheckedFileName);
-                const project = defaultProject || this.projectService.getDefaultProjectForFile(fileName, /*ensureProject*/ false);


Whats this change for ?

Fixes some code that was fishy under --strictNullChecks -- if the flag was passed this function returned a non-nullable type.

No! Earlier change was correct because we do not want to update graph to get project at this point. We would do that after checklist is completed.

sheetalkamat · 2018-06-18T21:08:06Z

src/server/editorServices.ts

-                if (!scriptInfo) {
-                    return Errors.ThrowNoProject();
+        getDefaultProjectForFile(fileName: NormalizedPath, ensureProject = false): Project | undefined {
+            const scriptInfo = this.getScriptInfoForNormalizedPath(fileName);


Do not change the public method's default here.

ghost · 2018-06-19T18:54:52Z

@sheetalkamat Please re-review

ghost · 2018-06-19T21:34:42Z

Latest commit fixes #25058

sheetalkamat · 2018-06-22T17:32:34Z

src/server/session.ts

+
+            const changes: (protocol.FileCodeEdits | FileTextChanges)[] = [];
+            this.projectService.forEachProject(project => {
+                for (const fileTextChanges of project.getLanguageService().getEditsForFileRename(oldPath, newPath, formatOptions, preferences)) {


Do you need to exclude orphan projects or the configured projects which have no open file?

Also the projects with language service disabled?

If the project has no files, getEditsForFileRename should just return nothing. Should getLanguageService() throw an error if the language service is disabled? Maybe we should have tryGetLanguageService() that returns LanguageService | undefined?

getLanguageSevice will return language service all the time but it will be disabled (cleared semantic cache. I am working on changes where language service itself will know that its disabled but for now this info is in project and we should ignore those projects.)
For orphan projects, we are going through them unnecessarily no? we could avoid all that work by just ignoring these projects

OK, we will now skip those projects.

ghost requested review from sheetalkamat, amcasey and mjbvz June 18, 2018 18:50

ghost force-pushed the getEditsForFileRename_project branch 2 times, most recently from 07e2ad8 to e6fd848 Compare June 18, 2018 19:16

Make GetEditsForFileRenameRequestArgs not extend FileRequestArgs

5369b37

ghost force-pushed the getEditsForFileRename_project branch from e6fd848 to 5369b37 Compare June 18, 2018 19:59

mjbvz approved these changes Jun 18, 2018

View reviewed changes

amcasey reviewed Jun 18, 2018

View reviewed changes

Code review: check new location first, and use scriptInfo.getDefaultP…

a8723a0

…roject()

sheetalkamat requested changes Jun 18, 2018

View reviewed changes

Andy Hanson added 2 commits June 18, 2018 14:49

Remove changes to e getDefaultProjectForFile (now #25060)

cf16bde

Undo API changes (#24966)

8eedd34

Combine edits from all projects (fixes #25052)

bb2e407

Andy Hanson added 2 commits June 19, 2018 14:52

Update API (#24966)

83e2a2c

Merge branch 'master' into getEditsForFileRename_project

2f2f328

sheetalkamat reviewed Jun 22, 2018

View reviewed changes

Ignore orphan projects or projects with language service disabled

5f749c4

sheetalkamat approved these changes Jun 25, 2018

View reviewed changes

ghost merged commit 878bf80 into master Jun 25, 2018

ghost deleted the getEditsForFileRename_project branch June 25, 2018 18:00

This was referenced Jun 25, 2018

getEditsForFileRename: Combine edits from all projects #25058

Closed

Support folder rename in getEditsForFileRename #24904

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make GetEditsForFileRenameRequestArgs not extend FileRequestArgs #25052

Make GetEditsForFileRenameRequestArgs not extend FileRequestArgs #25052

ghost commented Jun 18, 2018 •

edited by ghost

Loading

amcasey left a comment

amcasey Jun 18, 2018

amcasey Jun 18, 2018

sheetalkamat Jun 18, 2018

ghost Jun 18, 2018

ghost Jun 18, 2018

sheetalkamat Jun 18, 2018

amcasey Jun 18, 2018

ghost Jun 18, 2018

ghost Jun 21, 2018

amcasey Jun 18, 2018

sheetalkamat Jun 18, 2018

ghost Jun 18, 2018

sheetalkamat Jun 18, 2018

sheetalkamat Jun 18, 2018

ghost commented Jun 19, 2018

ghost commented Jun 19, 2018 •

edited by ghost

Loading

sheetalkamat Jun 22, 2018

sheetalkamat Jun 22, 2018

ghost Jun 22, 2018

sheetalkamat Jun 22, 2018

ghost Jun 25, 2018

Make GetEditsForFileRenameRequestArgs not extend FileRequestArgs #25052

Make GetEditsForFileRenameRequestArgs not extend FileRequestArgs #25052

Conversation

ghost commented Jun 18, 2018 • edited by ghost Loading

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 19, 2018

ghost commented Jun 19, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 18, 2018 •

edited by ghost

Loading

ghost commented Jun 19, 2018 •

edited by ghost

Loading