Don't offer `Convert to template string` refactoring for simple strings

[mjbvz]

TypeScript Version: 3.8.1-rc

Search Terms:

• refactor
• refactoring
• code actions

Repo
For code such as

import ImageEditor from 'tui-image-editor';

Place cursor on the string

Bug:
A convert to template string refactoring is offered. This is not helpful.

I suggest that we only show convert to template string if you are on a string concatenation expression.

[DanielRosenwasser]

So clearly this is silly because there are certain positions where a template string isn't valid - I think it'd probably be okay to disable this for simple strings, but we should revisit whether we want to enable it for all strings and disable it for nonsensical positions like

• import/export specifiers
• Property names
• Maybe other stuff?

[mjbvz]

I'd prefer to only see this refactoring when the cursor on a string concatenation expression

Also, we probably want to fix this for 3.8 (either for the final release or in a recovery build) as it is rather annoying to have the lightbulb show up for every string

[treybrisbane]

Just as a contrasting opinion, I actually find this behavior useful (in cases where template strings would be legal). After using TS/JS for years, my muscle memory now defaults to ${...} rather than + to template values into strings, and it's nice to be able to quickly convert the string in question to a template string in cases where it isn't already.

That said Daniel Rosenwasser (@DanielRosenwasser), another potential approach could be to make the suggestion if the string in question contains ${. Just a thought. 🙂

[nicoespeon]

Hey there 👋

A quick heads-up to support Trey Brisbane (@treybrisbane) point of view.

For the context, I'm the author of a VS Code extension that provides JS & TS refactorings. We used to provide "Convert to Template Literal". We removed it since it's now handled by the editor, through TypeScript.

However, it was very valid (and convenient) to switch from:

const a = "I have some apples";

to

const a = `I have some apples`;

To start inserting variables inside.

I understand this was dropped as a side-effect of a bug fix. I think what we really want to avoid is proposing the refactoring for invalid things like imports. I implemented that kind of check in my extension indeed.

You can check nicoespeon/abracadabra#94 for more context on this.

If you need more details, I'd be happy to help 😄
Thank you!