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

Implement on-type-rename for TypeScript #110573

Merged
merged 9 commits into from
Nov 17, 2020
Merged

Implement on-type-rename for TypeScript #110573

merged 9 commits into from
Nov 17, 2020

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Nov 13, 2020

This PR fixes nothing but explores on-type-rename for TS. There is no "proper" tsserver support for this but what I have seems to work surprisingly well.

@jrieken jrieken requested a review from mjbvz November 13, 2020 13:37
@jrieken jrieken self-assigned this Nov 13, 2020
@jrieken jrieken added editor-synced-region Issues related to synced region functionality in editor typescript Typescript support issues labels Nov 13, 2020
Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Neat! Change looks good to me overall

Should we ask TS for a proper API? Besides having to make two calls, are there any major issues with the current approach that TS could solve?

@mjbvz
Copy link
Collaborator

mjbvz commented Nov 13, 2020

One other question: does this work for renaming JSX elements? This was a highly upvoted issue: microsoft/TypeScript#51832

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

lllll`

@jrieken
Copy link
Member Author

jrieken commented Nov 16, 2020

fyi - I have also added type parameter to the list of kinds and I am skipping when there isn't at least 2 places to quick-rename.

One other question: does this work for renaming JSX elements? This was a highly upvoted issue: microsoft/TypeScript#51832

For JSX it seem that they are reported as property which is a little vague since that's also used for ordinary properties of TS/JS types. I don't know enough about JSX to know if that's enough or not but my understanding is that renaming those can potentially cause renamed in other files and that's currently unsupported

@yume-chan
Copy link
Contributor

One other question: does this work for renaming JSX elements?

Is it proper to call is "renaming"? Say I have two components: Bar and Baz, what mirror cursor can do is "change this Bar tag to be a Baz tag", not "rename Bar component to Baz", resulting two components both called Baz.

Looking at the implementation I think this pr is also doing the later

let bar = 1;
let baz = 2;
console.log(bar); // change this `bar` to `baz`, I don't want to rename the variable to `baz`, I just want to use `baz` instead

@jrieken jrieken merged commit f255e3e into master Nov 17, 2020
@jrieken jrieken deleted the joh/tsQuickRename branch November 17, 2020 09:46
@jrieken
Copy link
Member Author

jrieken commented Nov 17, 2020

Looking at the implementation I think this pr is also doing the later

Yes - as of today it is like f2-rename (which the name of the feature also implies). What I hear from you is to only rename usages and not the declaration in some cases. Unsure how such cases would be defined? Depending on where the cursor location is?

@yume-chan
Copy link
Contributor

yume-chan commented Nov 17, 2020

In Visual Studio and C#, after changing the name of something, there will be a code action to let you apply the edit as renaming, it will never do this by its own. (see #90361)

Maybe it's feasible to automatically rename something when cursor is on its declaration, but it's still possible that the user really want to do is replacing the implementation, so they rename the current one (as a backup), and add a new one with same name.

@jrieken
Copy link
Member Author

jrieken commented Nov 17, 2020

Maybe it's feasible to automatically rename something when cursor is on its declaration, but it's still possible that the user really want to do is replacing the implementation,

Sure, sure. That's why this is a feature that's off by default and also when its active you get clear feedback UX and can then cancel via ESC

@github-actions github-actions bot locked and limited conversation to collaborators Jan 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-synced-region Issues related to synced region functionality in editor typescript Typescript support issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants