Add opt-in user preference for prefix and suffix text on renames #29314

uniqueiniquity · 2019-01-08T23:26:59Z

Fixes #28679
This PR adds a user preference which, if not set, returns the rename behavior on shorthand properties and import/export specifiers to its state in 3.1.

amcasey · 2019-01-08T23:41:49Z

src/server/session.ts

@@ -1231,7 +1232,8 @@ namespace ts.server {
                this.getDefaultProject(args),
                { fileName: args.file, pos: position },
                !!args.findInStrings,
-                !!args.findInComments
+                !!args.findInComments,
+                this.getHostPreferences().usePrefixAndSuffixTextForRename || false


this.getHostPreferences().usePrefixAndSuffixTextForRename || false [](start = 16, length = 66)

Since there may be multiple preferences affecting rename behavior (e.g. #28677), would it make more sense to pass in the entire preferences object (e.g. as in getEditsForFileRename)?

Switched to passing entire preferences object.
However I only passed it as deep as this call; any deeper would require an API change, so I didn't want to do that unless absolutely necessary.

src/services/findAllReferences.ts

src/services/utilities.ts

src/compiler/types.ts

sandersn · 2019-01-09T00:10:35Z

Do you have a plan for when and how to remove this code? It's pretty ugly and I think it will only be needed for another 6 months or so. (Maybe more, depending on how long we ship new versions of TS to old versions of VS).

src/testRunner/unittests/tsserver/rename.ts

amcasey · 2019-01-09T00:14:50Z

@sandersn Don't we have to keep this until the newest version of VS without the opt-in is deprecated? I'm guessing that will be longer than 6 months.

sandersn · 2019-01-09T00:21:45Z

@amcasey Per our in-person discussion, we'll want to ship new TS servers to VS versions that shipped at least 2 years ago. (Plus we haven't done the work to make VS work with the new API anyway.)

sandersn

I take it you figured out how to make this work without changing getTypeAtLocation at all? If so, then sounds good. :)

uniqueiniquity · 2019-01-09T00:38:21Z

@sandersn yep, it was a red herring.

uniqueiniquity · 2019-01-15T19:47:06Z

@amcasey Can you take another look? I attempted to consolidate conditions like we discussed.

amcasey · 2019-01-15T23:12:08Z

        if (!(state.options.isForRename && (name.escapedText === InternalSymbolName.Default))) {

This is unaffected by the user preference?

Refers to: src/services/findAllReferences.ts:1092 in ad75ce7. [](commit_id = ad75ce7, deletion_comment = False)

amcasey · 2019-01-15T23:12:17Z

        if (addReferencesHere && !state.options.isForRename && state.markSeenReExportRHS(name)) {

This is unaffected by the user preference?

Refers to: src/services/findAllReferences.ts:1103 in ad75ce7. [](commit_id = ad75ce7, deletion_comment = False)

src/services/findAllReferences.ts

tests/baselines/reference/api/tsserverlibrary.d.ts

tests/cases/fourslash/findAllRefsShorthandPropertyFlag.ts

amcasey

Still a few questions but generally LGTM.

weswigham · 2019-01-15T23:46:48Z

Does this being a config setting mean it'll count as fixing #29238?

amcasey · 2019-01-15T23:53:09Z

@weswigham It means editors will have the option of exposing that as a setting but, arguably, it won't be fixed until they do.

weswigham · 2019-01-16T00:08:27Z

Since we've had a number of asks for #29238, we should try to make sure it's exposed as a setting in VS and VSCode, at least.

uniqueiniquity · 2019-01-16T00:10:56Z

@weswigham we can certainly plan to make it optional in VS. AFAIK @mjbvz has already made it the default in VS Code

uniqueiniquity · 2019-01-16T00:25:32Z

@amcasey Correct on your first comment, in no case do we want to rename the identifier default there. Also correct on your second comment, that behavior should only happen during a non-rename operation.

Porting #29314 to release-3.3

* origin/master: (64 commits) Fix resolution of properties from prototype assignment in JS (microsoft#29302) Include all flow nodes made within `try` blocks as antecedents for `catch` or `finally` blocks (microsoft#29466) Don't treat interfaces as implementations Make the relationship between partial mapped types and the empty object not apply for subtype relationship (microsoft#29384) Add missing arity check on second inference pass (microsoft#29386) renames add missing type annotation PR feedback Illustrate a case that isn't handled correctly Add fourslash tests Consider JSX namespace imports when moving statements between files Fix gulp builds not building some targets Update user baselines (microsoft#29444) Add opt-in user preference for prefix and suffix text on renames (microsoft#29314) Fake up value declaration for synthetic jsx children symbol so they get excess property checked (microsoft#29359) Add regression test. (microsoft#29433) Elaborate jsx children elementwise (microsoft#29264) Add regression test PR feedback Fix trailing whitespace ...

Fixes #29585. #29314 and #29385 made it so their respective settings are only recognized when provided to the host as a whole. This PR makes it so that the relevant settings for the preferences on the file override those of the preferences on the host.

justingrant · 2019-04-06T00:16:13Z

@uniqueiniquity - I just tried this new feature for the first time (thank you so much for building it!) and I was surprised that renaming a property of a named, destructured export wouldn't rename the symbol when it's imported in another file. Is this expected? I'll file a separate issue if not expected. Here's the repro:

Uncheck the "Rename Shorthand Properties" options for TS and JS
Create a new code file env.ts:

type ReadonlyAndNotUndefined<T> = {  readonly [P in keyof T]: string; }
export const {
  APP_NAME,
  MONGODB_CONNECTION_URI, 
  SENDGRID_API_KEY,
  NOTIFICATION_URL_BASE,
  NOTIFICATION_FROM_EMAIL,
} = process.env as ReadonlyAndNotUndefined<typeof process.env>;

Create another file, use_env.ts :

import { NOTIFICATION_URL_BASE, SENDGRID_API_KEY, NOTIFICATION_FROM_EMAIL } from './env';

In env.ts, use rename symbol (F2) to change the name of NOTIFICATION_FROM_EMAIL

Expected: name is also changed in use_env.ts
Actual: no change to use_env.ts, and a compiler error :

Module '"src/libs/env"' has no exported member 'NOTIFICATION_FROM_EMAIL'. Did you mean 'NOTIFICATION_FROM_EMAIL_2'?

Note that if you export individual variables (not via destructuring export) the rename works fine, e.g.
export const foo = 'bar'; can be renamed to foo2 and the rename will propagate to imports in other files.

Benjamin Lichtman added 11 commits January 8, 2019 14:15

Add user preference to control renaming through exports

c0c273d

Only impact renaming

d67899d

Update baselines

04153c3

Use flag to control all prefix and suffix text and imports

4d3d57c

[WIP] add tests

238ee41

Only skip export import specifier with flag

4332027

[WIP] Update tests

4e580ca

Update test

126cb76

Pick up preference from host and update test

18f503c

Shorten flag name

11a9d48

Add missing utility function

2ebd92b

uniqueiniquity assigned sandersn, sheetalkamat and amcasey Jan 8, 2019

Update comment

a29caef

amcasey reviewed Jan 8, 2019

View reviewed changes

src/services/findAllReferences.ts Outdated Show resolved Hide resolved

amcasey reviewed Jan 8, 2019

View reviewed changes

src/services/utilities.ts Outdated Show resolved Hide resolved

amcasey reviewed Jan 8, 2019

View reviewed changes

src/services/utilities.ts Outdated Show resolved Hide resolved

sandersn reviewed Jan 8, 2019

View reviewed changes

src/compiler/types.ts Outdated Show resolved Hide resolved

sandersn reviewed Jan 9, 2019

View reviewed changes

src/testRunner/unittests/tsserver/rename.ts Outdated Show resolved Hide resolved

sandersn approved these changes Jan 9, 2019

View reviewed changes

sheetalkamat approved these changes Jan 9, 2019

View reviewed changes

Benjamin Lichtman added 3 commits January 14, 2019 09:38

[WIP] rename flag and respond to cr

908217b

[WIP] Add flag for forRelatedSymbol

28bfc43

Use larger search symbol set for old-style rename

5c087ef

Benjamin Lichtman added 4 commits January 14, 2019 16:36

Respond to CR

087986e

Fix small error

b08c623

Merge branch 'master' into renamePrefixFlag

887b356

Fix type mismatch

306530c

Update comment and remove unnecessary exprot

ad75ce7

amcasey reviewed Jan 15, 2019

View reviewed changes

src/services/findAllReferences.ts Outdated Show resolved Hide resolved

amcasey reviewed Jan 15, 2019

View reviewed changes

src/services/findAllReferences.ts Outdated Show resolved Hide resolved

amcasey reviewed Jan 15, 2019

View reviewed changes

tests/baselines/reference/api/tsserverlibrary.d.ts Outdated Show resolved Hide resolved

amcasey reviewed Jan 15, 2019

View reviewed changes

tests/cases/fourslash/findAllRefsShorthandPropertyFlag.ts Outdated Show resolved Hide resolved

amcasey approved these changes Jan 15, 2019

View reviewed changes

Respond to CR

83049e8

uniqueiniquity merged commit 5fc8f1d into microsoft:master Jan 16, 2019

uniqueiniquity mentioned this pull request Jan 17, 2019

Port #29314 #29460

Merged

uniqueiniquity added a commit that referenced this pull request Jan 17, 2019

Port #29314 (#29460)

8680908

Porting #29314 to release-3.3

uniqueiniquity mentioned this pull request Jan 25, 2019

Allow per-file setting for rename default behavior preferences #29593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add opt-in user preference for prefix and suffix text on renames #29314

Add opt-in user preference for prefix and suffix text on renames #29314

uniqueiniquity commented Jan 8, 2019

amcasey Jan 8, 2019

uniqueiniquity Jan 15, 2019

sandersn commented Jan 9, 2019

amcasey commented Jan 9, 2019

sandersn commented Jan 9, 2019

sandersn left a comment

uniqueiniquity commented Jan 9, 2019

uniqueiniquity commented Jan 15, 2019

amcasey commented Jan 15, 2019

amcasey commented Jan 15, 2019

amcasey left a comment

weswigham commented Jan 15, 2019

amcasey commented Jan 15, 2019

weswigham commented Jan 16, 2019

uniqueiniquity commented Jan 16, 2019

uniqueiniquity commented Jan 16, 2019

justingrant commented Apr 6, 2019

Add opt-in user preference for prefix and suffix text on renames #29314

Add opt-in user preference for prefix and suffix text on renames #29314

Conversation

uniqueiniquity commented Jan 8, 2019

amcasey Jan 8, 2019

Choose a reason for hiding this comment

uniqueiniquity Jan 15, 2019

Choose a reason for hiding this comment

sandersn commented Jan 9, 2019

amcasey commented Jan 9, 2019

sandersn commented Jan 9, 2019

sandersn left a comment

Choose a reason for hiding this comment

uniqueiniquity commented Jan 9, 2019

uniqueiniquity commented Jan 15, 2019

amcasey commented Jan 15, 2019

amcasey commented Jan 15, 2019

amcasey left a comment

Choose a reason for hiding this comment

weswigham commented Jan 15, 2019

amcasey commented Jan 15, 2019

weswigham commented Jan 16, 2019

uniqueiniquity commented Jan 16, 2019

uniqueiniquity commented Jan 16, 2019

justingrant commented Apr 6, 2019