Excluded the default library from rename service. by armanio123 · Pull Request #17415 · microsoft/TypeScript

armanio123 · 2017-07-25T21:27:31Z

Excludes the default library when renaming tokens.

msftclas · 2017-07-25T21:27:35Z

@armanio123,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

billti · 2017-07-25T21:35:53Z

    class SourceMapSourceObject implements SourceMapSource {
        lineMap: number[];
-        constructor (public fileName: string, public text: string, public skipTrivia?: (pos: number) => number) {}
+        constructor(public fileName: string, public text: string, public skipTrivia?: (pos: number) => number) { }


All these white-space changes are interesting. Is it passing the linter? (Was it before?)

Yes it is passing linter (before and after). This was done by VS Code formatting, I thought it should be pushed.

billti · 2017-07-25T21:37:24Z

        }

+        function isSourceFileDefaultLibrary(file: SourceFile): boolean {
+            return file.fileName === host.getDefaultLibFileName(options);


I thought @rbuckton was factoring the core libraries into a set of smaller files that get added. If that's the case, I'm not sure this would work (as there wouldn't be just one default lib filename).

If that the case, we might want to change this method to check for all of the different files. I'll talk to @rbuckton and get his input.

Based on what I have discussed with @rbuckton the better way of doing this it to check for GetDefaultLibDirectory. Every file that is there is going to be considered core library.

Perhaps this should be named isSourceFileReadonly or isSourceFileLockedForEdits so that we can later use the same functionality to prevent renames in declarations pulled in from node_modules or other "typeRoots" locations.

billti · 2017-07-25T21:41:28Z

+            let sourceFiles: SourceFile[] = [];
+            if (options.isForRename) {
+                for (let sourceFile of program.getSourceFiles()) {
+                    if (!program.isSourceFileDefaultLibrary(sourceFile)) {


So previously if you tried to rename something that was defined in the core library (e.g. a JavaScript API such as Array.map, or a DOM API such as window.addEventListener), it would error because you can't rename something in the core library. With this change, will that rename now proceed, but only for all your program files (i.e. leaving you in an error state)? I'm not sure that's a better behavior.

That functionality will remain the same. That happens on the function getRenameInfo.
This piece of code specifically checks for the locations of the tokens (not the token type). It will only exclude the default library from being referenced when searching for the token to be renamed.

billti

I'd follow up with Ron on the core library not being one file piece, and Ryan on the behavior when renaming a core API now - but if they're happy, then looks good to me.

rbuckton · 2017-08-08T20:38:54Z

            synchronizeHostData();
-            return FindAllReferences.findReferencedEntries(program, cancellationToken, program.getSourceFiles(), getValidSourceFile(fileName), position, options);
+
+            // Exclude default library when renaming as commonly user don't want to change that file.


Will this block renaming a symbol declared in a default library? I would think that if a symbol is declared in a default library then we shouldn't allow the rename to proceed at all, rather than renaming it in a user's source files and resulting in possible compile time errors.

There's already a feature for that. This will avoid renaming anything on the default library, like comments or strings.

rbuckton · 2017-08-08T20:41:27Z

        }

+        function isSourceFileDefaultLibrary(file: SourceFile): boolean {
+            return file.fileName === host.getDefaultLibFileName(options);


Perhaps this should be named isSourceFileReadonly or isSourceFileLockedForEdits so that we can later use the same functionality to prevent renames in declarations pulled in from node_modules or other "typeRoots" locations.

…second library path and third name of file.

rbuckton · 2017-08-15T02:07:02Z

+                return true;
+            }
+
+            if (defaultLibraryPath !== undefined && defaultLibraryPath.length !== 0) {


This can be simplified: if (defaultLibraryPath)

rbuckton · 2017-08-15T02:08:03Z

+            }
+
+            if (defaultLibraryPath !== undefined && defaultLibraryPath.length !== 0) {
+                return comparePaths(defaultLibraryPath, file.path, currentDirectory, /*ignoreCase*/ true) === Comparison.EqualTo;


Do not unconditionally pass true for ignoreCase, instead use !host.useCaseInsensitiveFileNames().

I don't think comparePaths is the one you want to call. I think you meant to call containsPath here.

rbuckton · 2017-08-15T02:08:16Z

+                return comparePaths(defaultLibraryPath, file.path, currentDirectory, /*ignoreCase*/ true) === Comparison.EqualTo;
+            }
+
+            return compareStrings(file.fileName, host.getDefaultLibFileName(options), /*ignoreCase*/ true) === Comparison.EqualTo;


Do not unconditionally pass true for ignoreCase, instead use !host.useCaseInsensitiveFileNames().

rbuckton · 2017-08-15T02:10:56Z

        host = host || createCompilerHost(options);

        let skipDefaultLib = options.noLib;
+        const defaultLibraryPath = host.getDefaultLibLocation ? host.getDefaultLibLocation() : getDirectoryPath(host.getDefaultLibFileName(options));


There is also a libDirectory variable in this function that does the same thing. Consider combining the two and possibly storing the result of host.getDefaultLibFileName(options) in a variable rather than requery it each time.

rbuckton

Approved, with a minor comment.

rbuckton · 2017-09-14T18:33:46Z

        host = host || createCompilerHost(options);

        let skipDefaultLib = options.noLib;
+        const getDefaultLibraryFileName = memoize(() => host.getDefaultLibFileName(options));


Why bother with memoize and not just have const defaultLibraryFileName = host.getDefaultLibraryFileName(options)?

Depending on the options and some logic, the variable might end up not being needed. This was done for lazy evaluation.

mhegazy · 2017-09-18T20:36:53Z

I do not think this is the best solution here. the default libraries are not any special. users can replace them. and the same issue persists if some one say renamed div in a JSX tag.

A better solution is to block rename for symbols coming from a .d.ts file unless the source of the rename operation is that file. see #8227

armanio123 requested review from RyanCavanaugh and billti July 25, 2017 21:27

msftclas added the cla-not-required label Jul 25, 2017

billti reviewed Jul 25, 2017

View reviewed changes

rbuckton suggested changes Aug 8, 2017

View reviewed changes

Armando Aguirre added 2 commits August 14, 2017 17:58

Excluded the default library from rename service.

5b86055

Fixed failing tests and lint. Added unit tests.

675b6fb

armanio123 force-pushed the FixRenameInDefaultLibrary branch from 6cc5710 to 675b6fb Compare August 15, 2017 00:59

Changed check for default library by first checking hasNoDefaultLib, …

88262db

…second library path and third name of file.

rbuckton suggested changes Aug 15, 2017

View reviewed changes

Added host.useCaseSens..., memoized getDefaultLibFileName.

9dd574b

rbuckton approved these changes Sep 14, 2017

View reviewed changes

Armando Aguirre added 2 commits September 14, 2017 12:01

Merge branch 'master' into FixRenameInDefaultLibrary

b03fbaf

Fixed sourceFiles type error

1ab67c0

armanio123 merged commit 21bbdd3 into microsoft:master Sep 14, 2017

armanio123 deleted the FixRenameInDefaultLibrary branch September 14, 2017 21:04

microsoft locked and limited conversation to collaborators Jun 14, 2018

Conversation

armanio123 commented Jul 25, 2017

Uh oh!

msftclas commented Jul 25, 2017

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!

armanio123 Jul 25, 2017 • 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!

billti left a comment

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!

rbuckton left a comment

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 Sep 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

armanio123 Jul 25, 2017 •

edited

Loading