Inline variable from identifier should inline its reference

[martixy]

Describe what's missing

The extension seems to inline the top-most variable, instead of the selected variable.
(Initially I thought this was a bug, but it turned out to be a difference of expectations.)

How to reproduce

Try to inline doink by selecting it's usage in waffles.

const doink = () => import('@/views/Orders/Orders')

const waffles = [
  {
    thing: doink
  }
]

const levelup = [
  waffles
]

Expected behavior

const waffles = [
  {
    thing: () => import('@/views/Orders/Orders')
  }
]

const levelup = [
  waffles
]

Current behavior

const doink = () => import('@/views/Orders/Orders')


const levelup = [
  [
  {
    thing: doink
  }
]
]

Additional information

And it doesn't indent things properly.

• Version of the extension impacted: v4.12.1

🧙‍ Add any other context about the problem here.
I first considered this a bug, but looking at it more, it seems to be a difference of expectation. When I select the usage of a variable, I expect to inline that, and all other usages.

[nicoespeon]

Thanks for the suggestion. It's a very reasonable expectation indeed.

The behavior depicted here isn't much the result of a conscious decision, but a fallback. Today, the refactoring doesn't work from the variable identifier (doink). You need to be on the variable declaration to do so. But because this is triggered from an existing variable declaration (waffles), then it inlines the latter instead of the former.

This would be a nice improvement.

I anticipate we'll need to handle some questions:

• We should go for the narrowest scope when there's a conflict between an identifier and a variable declaration (like in the example)
• From an identifier, we may expect to inline only this reference and not all of them
  • Should we still ask the user or go for the SingleOccurrence scenario?

[martixy]

Q1: I'm not sure about the mechanics of what happens (tokenization, etc.), but... purely from the standpoint of how a human parses it, you select the identifier, and expect that identifier to be replaced with whatever it points to.
Q2: a) Having an option is the most user-friendly approach. b) I can imagine a scenario where if the reference points to a literal(e.g. a string), you may wish to replace only this one reference. However I'm not sure creating different behaviours based on what the variable contains is a terribly good idea.

The questions seem a bit too tied to implementation details for me to offer anything substantial, but I can say this: This is not reinventing the wheel. Other IDEs exist out there with similar functionality. You can examine how the feature works there and copy it. So the answers have already been determined, should you decide to seek them out. And copying behaviour from other places has the added benefit of avoiding mismatched expectations. One place where I know this already works is in Jetbrains IDEs.

[ehaynes99]

Regarding the question of scope, I think it's simpler than that; I would not expect it to do anything unless the cursor is touching a variable name. Trying to infer which variable to associate with every random cursor position in the document would be chaos, and nested scopes would make it a coin toss.

I've forgotten practically everything about Jetbrains' Intellij except this feature and how the diff editor had copy left/right. It did this one really well, though.
https://www.jetbrains.com/help/idea/inline.html#inline_variable

It still gets complicated if there is more than one usage. Theirs pops up a dialog that asks if you want to inline just that one, all of them and remove the declaration.
https://resources.jetbrains.com/help/img/idea/2020.3/ij_extract_method_5_inline.png

Theirs goes further with methods, but I wouldn't even attempt to cover the cases of object properties or methods in JS, as that opens the can of worms for when those objects are passed out of the scope of the current file. Theirs does search the whole project and do the refactor, but at least years ago when I used it, it did so horribly for languages that weren't compiled.