Typescript: Option to disable aliased JS/TS destructured property renaming

[jasongerbes]

As per the version 1.30 release notes:

Renames handle JS/TS destructuring properly
Renames now handle JavaScript and TypeScript destructuring and will introduce an alias if needed

This means that renaming a property will result in the property being aliased by it's old name wherever destructuring assignment has been used. For example:

// before rename
const x = {
    y: 'value'
}
const { y } = x;
console.log(y);

// after rename
const x = {
    z: 'value'
}
const { z: y } = x;
console.log(y);

Previously, the destructed parameter's name would be updated, as well as all of its usages:

// before rename
const x = {
    y: 'value'
}
const { y } = x;
console.log(y);

// after rename
const x = {
    z: 'value'
}
const { z } = x;
console.log(z);

Can you please make this new behaviour optional?

[mjbvz]

Related: #29026 and possibly #28679

[ghost]

Yes this is madness, because now I have these aliases all over my codebase when I do a rename, because when I rename it affects lots of code.
A default behavior is always a compromise, but should be more sensible then not. And this isn't the case here.
Oh yes and reverting to 3.1.6 where renaming is actually usable :)

[zpdDG4gta8XKpMCd]

please oh please let us do aggressive renaming up to the declaration of a symbol

[hippostar]

I thought this was a bug, the new way it adds a bunch of aliases is completely unusable. Please revert it to the way it was, or let us disable this new behaviour.

[purpledrgn]

If you have a ton of files and you want to rename the symbol in all of them, you do not want to go though Ctrl+H or each one, one by one. This "feature" is terrible, and is also too different to how every other editor does it.

It's also counter intuitive to the name. It's called "Symbol Rename" not "Add/Rename Local Alias"

[deryost]

Me and all my teamates founded this awful !

[quentez]

I agree that this should be a preference. What used to be quick rename refactors are now taking us a lot more time as we need to make sure we don't forget to remove any of the aliases that were created in the process.

[ratbeard]

I ended up downgrading to 1.29 by downloading here and setting update.channel: 'none' in my settings to turn of auto updating to get this back 😦

[svipas]

I thought this was from VS Code and I created an issue. With all the respect for TS team, this feature is really a pain in the ass. Now after renaming the property, I need to go through all the files to remove an alias by myself. Please, revert to the old behavior like it was before. Or make it optional and let us decide how we want it to be. Thank you.

[RyanCavanaugh]

There are three cases to think about here

const x = {
    y: 'value'
//  ^- rename at key
}
const { y } = x;
//      ^- rename at declaration
console.log(y);
//          ^- rename at use

A general thing we should agree on as a first principle is that refactoring should semantically change your program as little as possible.

Renaming the property in the object literal is a huge semantic change to your program in service of a refactoring (renaming a local) which requires no semantic changes whatsoever. This could easily break your program at runtime when all you wanted was a different variable name - not good.

A rename at a use of the local is extremely defensible to implement as an aliasing under this rule.

Renaming at the declaration is arguably the closest thing to a "declaring reference" of both ys that you could get, and it seems reasonable to not introducing an aliasing in this case.

Renaming at the key, we have to also consider this case:

const x = {
    y: 'value'
//  ^- rename at key
}
const { y } = x;
const k = { y };

It's extremely unclear whether you intend for k.y to be renamed as a result here. The safe assumption is "no" and it seems preferable to introduce an aliasing, but it's not even clear which y should be aliased - I would argue the declaring y should be aliased to keep the rename effect as local as possible.

Thoughts?

[zpdDG4gta8XKpMCd]

practically speaking i only care about a handful places in my application where i don't want to rename accidently:

1. libs.d.ts, jquery.d.ts, ....
2. autogenerated contracts with the backend
3. interfaces of the configs of the users that were saved in the database and won't be read properly if i rename anything

anywhere else i wish i could rename freely

so it comes down to:

• allowing aggressive renaming anywhere
• except for places that are purposefully excluded

so we need either of the following:

• new section in tsconfig
• design time attributes
• jsdoc tags of special purpose

to indicate what should not be renamed

[zpdDG4gta8XKpMCd]

... or all sufficient workaround to how it currently works by

• allow renaming at declaration sites only which means that by pressing F2:
  
  
  
  1. we first get navigated to a declaration
  
  
  2. a prompt for renaming is shown
  
  

[jasongerbes]

I can understand the logic behind this change and it being the default behaviour.

What I do have an issue with is that there is no way to enable the existing behaviour that at least the people in this thread prefer.

[purpledrgn]

@RyanCavanaugh

There are three cases to think about here

const x = {
  y: 'value'
//  ^- rename at key
}
const { y } = x;
//      ^- rename at declaration
console.log(y);
//          ^- rename at use

A general thing we should agree on as a first principle is that refactoring should semantically change your program as little as possible.

First, I hope you realize the irony in telling people that something should change as little as possible, _while breaking the established mental model with this change_.

This change is too defensive. Being defensive is not always the correct approach.

The only smart thing it needs to do, related to destruction and object notation shorthand is that if a symbol is renamed and the symbol was/is used in constructing the key should be added in, ie. { name } becomes { name: givenName }. But, that's only when the symbol is used to build an object not when it's the destructed property of one.

Even more so for exports from modules (including require). They may or may not be implemented as a destruction but as far as I'm concerned that's just the syntax for importing the 3rd party symbol and _it just happens to kind-of look the same_ with the destruction syntax. It's not like any of us have a choice in this. If I don't explicitly alias it when importing I don't expect it to be treated as a different symbol, just as I don't expect with regular require let { Something } = require('example') and require('example').Something to be different symbols as far as typescript is concerned.

With regard to destruction, the only case where the symbol would be considered different from the thing that's being renamed is when have an explicit alias in place, eg. let { name: personName } = person;

The new behavior, "alias symbol at closest reference" should be something like Ctrl+F2. It's fine if you want the new behavior on F2 and old behavior moved to Ctrl+F2, just add a warning when people use it the first time so they don't think you completely changed it.

It's _extremely_ unclear whether you intend for k.y to be renamed as a result here. The safe assumption is "no" and it seems preferable to introduce an aliasing, but it's not even clear which y should be aliased - I would argue the declaring y should be aliased to keep the rename effect as local as possible.

Thoughts?

With regard to the philosophical question, of which is better. The answer is none of them is better. Arguing over them is like arguing over different sized screw drivers and different sized hammers. You sometimes need a small hammer, for small things, and you sometimes need a really big hammer because nothing smaller will do.

I kind of understand what you went for here, where the level of refactor is I assume based on the location. But this is just confusing and given that I've had to come here and read about it, unintuitive. How am I suppose to know that if I hit F2 on the same thing somewhere else in code it would have different effect?

The correct behavior if you want to have all of them under the same key is to have a popup. However that's annoying for rudimentary cases. When developing something new it's very easy to have to do a lot of renaming, same if you're lazy prototyping then moving to more serious version. Naming things is hard, but good (global) refactoring makes it a painless process....... _or at least it was in previous versions_.

I have been using the symbol rename as present in previous versions (with various IDEs) for years now with out issue. Yes, with most implementations, it does potentially cause some errors to popup when the system just doesn't know what's best and simply does the rename as requested (as is sensible, given it's name). This has never been an issue, just fix the errors. For your alias use case, since it always involves just the one file Ctrl+H stepping though all occurrences works fine--and is actually more flexible since it covers the cases where "it's the same thing, but declared as a variable in different functions/code-blocks".

I appreciate the thought, but this alias feature is just not very useful to me personally and just makes my life much harder, sorry.

[RyanCavanaugh]

A per-user setting is being implemented at #29593

[fatcerberus]

const x = {
    y: 'value'
//  ^- rename at key
}
const { y } = x;
//      ^- rename at declaration
console.log(y);
//          ^- rename at use

The "rename at use" case is the most interesting - in that specific case I would definitely prefer the alias over renaming the object property itself (especially since I may not even realize that's going to happen depending on how far away the declaration is at that point--I don't know or care whether the variable came from a destructuring or not, I only want to rename it locally).

In all other cases, no, I'll agree with everyone else in the thread that creating an alias by default is not acceptable.

[dawnmist]

Where I keep running into it is with declaring React Component Props interfaces.

If I rename the variable in the interface, it's usually because I have thought of a more descriptive/better name for it. In that case, I absolutely _do_ want the uses of that prop inside the component to also be updated to the new name, and for external items to pass the data to that component under the new name, as it better documents what the data should be. Right now it's greatly slowing me down any time I try to update the props for a component that I'm still actively developing/getting feedback on/etc.

I'm really glad to see that a per-user setting is being implemented, as I came here today specifically to look at how to turn the behaviour off (after trying it for several weeks and getting more and more frustrated).

I can understand the reasoning behind the change. However in practice over the time I've been using this I _haven't come across a single case_ where this behaviour was actually desired/welcome. I have come across multiple cases on a daily basis where this behaviour was the opposite of what was desired (i.e. the old behaviour was what was needed).

[RyanCavanaugh]

See above comments / linked issues

[pie6k]

Is it released already?

[purpledrgn]

@pie6k don't think so, still on 1.29.1 personally.

[another-guy]

@jasongerbes

Can you please make this new behaviour optional?

👍 and disabled by default because it's not a meaningful default in many people views. (Yes, I read Ryan's explanation on safety of the current default approach, but I'm still not convinced that safety overweights the pain it comes with. Especially if we think about the likelyhood of the issue caused by an unsafe rename).

[fatcerberus]

@another-guy https://github.com/Microsoft/TypeScript/pull/29593 - this PR was already linked above.

A per-user setting is being implemented at #29593

[OliverJAsh]

A per-user setting is being implemented at #29593

This is now available in VS Code, thanks to https://github.com/microsoft/vscode/issues/68029.

{
  "javascript.preferences.renameShorthandProperties": false,
  "typescript.preferences.renameShorthandProperties": false
}

[dsherret]

@OliverJAsh worth noting that changing that setting will also add aliases to export and import specifiers—it changes more behaviour than it says.

[OliverJAsh]

@dsherret Do you mean that will happen if you keep the setting enabled (it's true by default)? That's what I'm seeing.

Sometimes I want aliasing, sometimes I don't (e.g. imports), so I opened a new issue to suggest that we allow users to control what happens at the time they actually perform the rename: https://github.com/microsoft/vscode/issues/93501