fix #10184: Recognize assigment in v-if and do not throw warning

[grandsong]

The original issue #10184 is too long, and I've made some mistakes when writing it.

Here, I will rephrase the issue, and explain my solution.

What problem?

Demos

Here are two demo for comparison.

1. Vanilla vue.js warns when assigment in v-if happens
2. My modified vue.js doesn't warn when assigment in v-if happens

Remember to open Dev Tools and check the Console.

The code in question is <span v-if="pet = person.pet">.

What happens in vanilla vue.js?

vue.js doesn't check the content of the expression of a v-if.

To it, pet = person.pet is just an arbitrary expression.

It desn't care whether assigment happens in a v-if.

All if conditions will be compiled as ternary statements.

In the example, the compiled codes will be like:

((pet = person.pet) ? `${person.name}'s pet's name is ${pet.name}.` : `${person.name} has no pet.`)

As a result, while the template is being rendered, at the beginning when the renderer meets this identifier pet, the proxy of vue instane (let's name it as vm) will automatically try to get pet as a property of the instance.

Of course it will fail. vm.pet has not been defined yet.
So, a warning will be thrown.

[Vue warn]: Property or method "pet" is not defined on the instance but referenced during render.

Then, the renderer meets = person.pet, so it assigns the pet as a property of the vue instance, namely, vm.pet = person.pet.

After that, when it meets pet again in pet.name, it gets pet (namely, vm.pet) successfully.

If we would manually do the rendering in place of vue (without the help of the magical proxy and its get-handler), we will deal with codes like this:

((vm.pet = person.pet) ? `${person.name}'s pet's name is ${vm.pet.name}.` : `${person.name} has no pet.`)

A spoiler: person is different. It doesn't bother the proxy.

Why do we need assigment in v-if?

Why do we need any shorthand (like : for v-bind:), or sugar syntax (for in instead of for(var ..;..<..length;..++)), etc?

Making life easier!

We can live with inconvenience. But we still ask for better life when we have chance.

The above demos

Without the assigment, the codes would become

<span v-if="person.pet">{{person.name}}'s pet's name is {{person.pet.name}}.</span>

Well, looks completely tolerable...

But, what if the design is more complex?

<span v-if="person.pet">
	<span v-if="person.pet.hunger > 0">
		{{person.name}}'s pet's {{person.pet.name}} needs to eat
		<span> {{foodDict[person.pet.favFoodId]}}</span>
		, which
		<span v-if="foodQtyInStore[person.pet.favFoodId]"> you have {{foodQtyInStore[person.pet.favFoodId]}}.</span>
		<span v-else class="color-warning"> is out of stock!<span>
	</span>
</span>

Compare it with

<span v-if="pet = person.pet">
	<span v-if="pet.hunger > 0">
		{{person.name}}'s pet's {{pet.name}} needs to eat
		<span> {{foodDict[pet.favFoodId].name}}</span>
		, which
		<span v-if="foodQty = foodQtyInStore[pet.favFoodId]"> you have {{foodQty}}.</span>
		<span v-else class="color-warning"> is out of stock!<span>
	</span>
</span>

slotScope

And let's take another example - an example in the source of vue.js itself:

	function processSlotContent (el) {
	  let slotScope
	  if (el.tag === 'template') {
	    ...
	  } else if ((slotScope = getAndRemoveAttr(el, 'slot-scope'))) {
	    ...
	    el.slotScope = slotScope
	  }
	  ...
	}

(It purposely uses double brackets to fool lint-flow-types, which forbids assigment in if.)

Without the assigment, it would become

	function processSlotContent (el) {
	  let slotScope
	  if (el.tag === 'template') {
	    ...
	  } else if (getAndRemoveAttr(el, 'slot-scope')) {
	    ...
	    el.slotScope = getAndRemoveAttr(el, 'slot-scope')
	  }
	  ...
	}

I don't know if it is OK for getAndRemoveAttr(el, 'slot-scope') to be called twice.
Probably not.

Maybe we still have to use assigment, just not in if-condition.

	function processSlotContent (el) {
	  let slotScope
	  if (el.tag === 'template') {
	    ...
	  } else {
		  slotScope = getAndRemoveAttr(el, 'slot-scope')
			if (slotScope) {
		    ...
		    el.slotScope = slotScope
		  }
		}
	  ...
	}

if let

Another thing we can analogize to is the if let in Swift.

See Optional Binding if you haven't learnt it or don't quite recall.

Before I learnt this feature of Swift, I was very reluctant to assigment in if-condition.

Because it would remind me of my several stupid mistakes of = for == as well as my old time as a JS newbie.

I would rather write more codes (like the slotScope one) to avoid assigment in if-condition.

But Swift gave me a good lesson.
Smart people should embrace useful features when opportunity allows (designing a new language).

What about danger?

In pure JS, thanks to js linters, assigment in if-condition is not as dangerous as it used to be. Mistakes of = for == can be easily detected.

(That's why #7631 warn when assignment in v-if in development mode was closed by @posva.)

Then, what's wrong with vue?

In vue, it is another story.

There is no way to easily create local variables yet.

So, even if I just want assignment outside v-if, I cannot assign at all.

(There is a solution for slot-scope. But I seldom use slots.)

Whether we deserve such freedom is beyond this issue. Let's leave it and focus on assignment in v-if.

Please remember these fact:

• assignment in v-if already works

• the warning thrown is not very helpful

The warning is thrown, not because vue has a rule which forbids assignment in v-if, but because vue fails to recognize the identifier as a new one, which of course is not defined.

And, bug?

And there can be possible conflict (I don't know if it can be called a bug/loophole):

If somehow the developer writes

	data() {
  return {
    b: false,
    pet: {
    	// some other pet
    	name: 'Tom'
  	},
    persons: [...]
  }
}

Now, there is vm.pet (Tom) before the template is rendered.

So, when the renderer meets v-if="pet = perons.pet", it will happily get it!
And then, vm.pet will be overwritten and Tom will no longer exist!

Compare v-for

v-for naturally contains assignment.

Although not mentioned in the doc, vue actually even "support destructuring assignment". It is a little secret.

So, what goes on with v-for? Why identifiers in it doesn't lead to warnings?

The answer is simple: compiled codes for v-for are functions, so that the identifiers are local varibles within the functions.

That's why you dont' see vm.person.name in the "If we would manually do the rendering" code:

((vm.pet = person.pet) ? `${person.name}'s pet's name is ${vm.pet.name}.` : `${person.name} has no pet.`)

This inspired me when I was trying to make a solution.

What solution?

Simple.

You can see that I added/modified only very few lines below.

Quick explanation:

1. In "parser/index.js", assignmentInIfRE is define.

2. In function addIfCondition, assignmentInIfRE is used to find the identifier, and store it as condition.alias.

3. In "codegen/index.js", save the original compiled result to ternaryExp.

4. If condition.alias exists, add a declaration of the identifier. Then, wrap the codes as an IIFE (Immediately Invoked Function Expression).

With IIFE, it works in a way very similar to the optional binding (if let) in Swift.

BTW, Why alias?

The parser for v-for stores identifiers (i.e. person) as alias of the node.

I don't know what uses are of these aliases, but I like the choice of name.

Hey, no test yet? How dare you!

Sorry.
I tried to follow the instruction but I failed to do the test.
Twice.

I run yarn, then I got stuck. It shows several "waiting..." forever.

I had to stop it manually, and a rerun was no luck either.

Anyway, I've already made a working modified vue.js, in my Demo 2.

So I guess it is highly likely for my simple solution to run well with this pull request, too.

Besides, there might be problems that I cannot anticipate.
Today it was the first time I've delved into the source of vue.

Real experts may be needed for polishing this feature.

---

PULL REQUEST TEMPLATE

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Build-related changes
• Other, please describe:

Does this PR introduce a breaking change? (check one)

• Yes
• No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

• It's submitted to the dev branch for v2.x (or to a previous version branch), not the master branch
• When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
• All tests are passing: https://github.com/vuejs/vue/blob/dev/.github/CONTRIBUTING.md#development-setup
• New/updated tests are included

If adding a new feature, the PR's description includes:

• A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

[grandsong]

The updates were just to meet the strict demands by lint-flow-types of CI.

[posva]

While I appreciate all the thoughts you have put into this as well as the detailed explanation, declaring variables in a v-if is not something we want to support because most of the time, it's a mistake of using = instead of == or ===, so we want to keep the warning working as it is.

If what you are looking for is a way to declare variables, there was a proposal #7325 but it was cancelled in favor of using scoped slots. So instead, I recommend you to use something like https://github.com/posva/vue-local-scope

It looks like you have put a lot of work in this, I'm sorry it didn't get merged at the end. To avoid this to happen again, I would go by the rule of never implementing a feature of a closed issue

[grandsong]

You kept telling me that you have a ready tool.

I replied you in a comment of my issue, and in this PR I aslo wrote:

There is no way to easily create local variables yet.

(There is a solution for slot-scope. But I seldom use slots.)

Whether we deserve such freedom is beyond this issue. Let's leave it and focus on assignment in v-if.

The slot-scope is very useful, only for slot users.

But why will I choose to add slots, and thus change my template nesting relations, when I just want to make my codes shorter and neater?

For now, I always "bake my date beforehand". It is OK. It is the best "alternative" before this PR is accepted.

In some other cases, I will even choose vue components, when reusability prevails.

Slot is the least thing I have used. (I've just used twice.)

--

I am not proposing support for arbitary local varibles.

I have no intent to extend the topic to that far.

I don't think the need is very common (if in the original design, no slots are needed at first place)

Even if a way is provided to do that, it will require developers

1. to learn a new syntax (or even more), and
2. to add some codes, and thus the net benifit of "making codes shorter" will be offset, from small to tiny.

Not very worthy.

In contrast, assigment in v-if requires no learning, no extra codes.

It already works, because it is part of JS language. No more, no less.

--

It has been such a frustrating and dissappointing experience for my first attempt to contribute vue!

I admit my writing of my issue was not very good, in which I probably misled readers.

And my words are too long for people to give enough patience, which has becoming more and more scarce nowadays.

My bad. So I completely rephrased the issue in this PR in hope of better understanding.

This RP is still long, but it introduces and digs the topic in a gradual and organized way.

However, my effort is wasted.

Your reason for objection seems to be a result of over-simpilifying the issue.

No matter how I discussed it over and over again.

I had predicted this very reason at the beginning of my thinking before I decided that I should propose a worthy request.

Then I have put a good lot of words about it as well as why I believe we should not be hindered by it.

--

Besides, it is really strange that YOU NOW use this reason after you denied the very similar idea of #7631: "warn when assignment in v-if in development mode".

Let me quote what you said:

this is a js common pitfall for beginners, not Vue. It should be said on your editor by a linter (https://github.com/vuejs/eslint-plugin-vue)

I mentioned this issue and your comment in my issue.

In this PR, I also emphasized the same belief as yours, by writing a small section:

What about danger?
In pure JS, thanks to js linters, assigment in if-condition is not as dangerous as it used to be. Mistakes of = for == can be easily detected.

It is linters's, NOT vue's, job to "teach" people how to write js in good styles.

And style rules can vary by different teams. That's why linters have tons of options about rules.

When a developer writes an assignemnt in if condition, how the hell a machine can tell if they are making a mistake or they are clearly knowing what they are doing?

And if the machine is smart enough, it should be able to know when the codes are ACTUALLY correct, instead of throwing warnings mindlessly.

--

True. It is much more dangerous to ride a motorbike than car.

So every rider should be warned every time they need to take a short and safe trip?

Everyone should be encouraged to choose four-wheeled vehicles while all two-wheeled ones should be always discouraged?

I myself had been such a puritan of strict code styles. And I still am, mostly.

I've just changed my view about this specific part: "assignment in if condition is evil".

I believed so. As a result, I've written many, many var x = some_long_exp; if(x){...}.

I mentioned if let in Swift, because it was the cause of my mind shift, making me reexamine the idea more deeply.

Newbies need warnings to avoid stupid mistakes which they are prone to making.
Veterans need warnings too, in case they do again occasionally.

The only difference is that the latter are capable of going on beyond the warnings, as long as they are intentionally creating better codes than safe but elementary ones.

In this PR, I talked about a very interesting case from the source of vue itself!

else if ((slotScope = getAndRemoveAttr(el, 'slot-scope')))

I then analysized why the coder chose to write so:

1. getAndRemoveAttr probably should be called only once. So assignment is necessary.

2. Assigment to slotScope outside if-condition would be longer and less elegant.

He (or she) even purposely uses double brackets (a trick) to fool lint-flow-type!

Was he wrong to do so? Should this code be "corrected"?

Unless there is an official rule against this style, I believe he has done a very good job.

--

PS:

I made this PR after my attempt to prove @Justineo wrong. He said

it's not practical for Vue to parse and find assignment expressions inside the whole v-if expression

I doubted it, so I mustered the courage to delve into the source of vue.

Then surprisingly I finished my job, far more quickly and successfully than I had expected.

Just a few lines.

It turned out that it IS practical, very much so.

"Talk is cheap." Indeed.

[grandsong]

[IMPORTANT UPDATE]

I have a new study on the usefulness (uselessness) of the current warning:

It does not help ANYONE!

For example:

With v-if="animal = person.pet",

There can be two cases:

1) animal is not defined.

This is my case.

A warning is thrown, but I don't want it.

2) animal is defined, in data.

(animal is at as the same level of persons. vue renderer accesses them as properties of the vue instance.)

That is the case of #7631.

And then, NO warning is thrown at all!

vue renderer just overrides the property animal, leaving the program buggy!

People who make mistakes of = for == still suffer!
Yes, because you closed #7631.

So my PR is completely irrelevant here!

It won't make lives of mistake-makers become better or worse.
Just IRRELEVANT.

[Justineo]

But it's not practical for Vue to parse and find assignment expressions inside the whole v-if expression because it can has arbitrary complexity.

You've missed the most important part. For v-for, we've claimed that it accepts a micro syntax of alias in expression, which makes it trivial to recognize each part with a regular expression. While for v-if, it accepts any valid JavaScript expression. In JavaScript you can't declare a local variable in if conditions. What you propose is to introduce a new micro syntax here and by doing so, it means using assignment in v-if is kind of encouraged. But this lease quite a lot footguns and users might shoot themselves.

Some obvious mistakes to you may confuse others. No mention a simple regular expression cannot handle all possible cases.

Consider the following cases:

v-if="item = item[i]" works fine and later someone want to add a condition and writes v-if="item = item[i] && item[i + 1]".

Newcomers may have a hard time figuring out the difference among:

v-if="foo = items[i] && bar" 
v-if="foo && bar = items[i]"
v-if="foo = item[i].foo && bar = item[i].bar"

And as we support arbitrary JS expression in v-if, you can't stop people write v-if="foo === 'bar = b'" && bar which will definitely a false-positive match of the regular expression. That's what I mean by saying “not practical”: regular expressions aren't robust enough for this.

Moreover, this implicit declaration also changed current semantics: v-if="foo = bar" use to be interpreted as this.foo = this.bar ? result1 : result2, implicitly change it to (function (){ var foo; return foo = this.bar ? result1 : result2 })() is actually a breaking change.

Changing the code is easy, but we need to consider all possible corner cases and outcomes it may produce.

[Justineo]

It does not help ANYONE!

Not exactly.

1. animal is not defined.
   This is my case.

A warning is thrown, but I don't want it.

For people who aren't intentionally using assignment and expect to declare something new, but by mistake, it helps.

[grandsong]

Why did you stop reading just before I talked about exactly what you thought to be a reason of objection??????????

[grandsong]

Thank you for serious discussion, which is the very thing I had been long anticipating / hoping for.

You pointed out some concerns either that I didn't think of, or that I failed to elaborate enough my reasoning for.

However, I want to discuss more about why I cannot completely agree with you.

About "footguns"

Please read the second part of my last comment.

People can "shoot themselves" when using vue, NOW, because no warning will be thrown when they make such mistakes.

This was the topic of #7631, not mine.

(It was too short and simple, and failed to point out what true problem is like. But the problem is still out there. I suggest that issue be reconsidered.)

Anyway, my issue is very different, more than it appears.

Loophole

I didn't want to call it "loophole", but in a sense, it is.

People can write assignment in v-if in vue, now.

Just like they can in pure JS.

Yes, there is no if let (nor if var) in pure JS.
But we can write

function test(){
	var person = {pet: {name: 'Tom'}}
	if(animal = person.pet) console.log(animal.name);
}
test()
// Tome

As a result, we will get window.animal.

The similar thing goes here in vue. we will get vm.animal (vm is the instance of vue).
Despite the warning, vue goes on.

It is not good, of course. But, it works.

I don't know, maybe there won't be warning in production mode.
So some people will use it as a "trick".

I myself feel it very tempting to use the "trick".
It is so helpful when I am in the situation which urges for it.

That convenience is like what if let in Swift provides its users.
When oppurtunity is at hand, why don't try?

Since we now have discovered this loophole (or whatever we call it), we'd better not turn a blind eye to it.

There can be two decisions:

1. Accept this, let it continue to work, and make vue better.
2. Deny this, and maybe "fix" it so that no "trick" can be allow.

[grandsong]

"foo = item[i].foo && bar = item[i].bar"

We can make vue recognize these identifiers all.

Just a little modification of the codes.
Two lines.

v-if="foo === 'bar = b'"

... Eh, what?
Oh...!

Yes, you gave me a really powerful hit!

I have to admit this proves the limits of regexp, which I hadn't thought of.
It beats me.
It alone can kill this PR.

Congralutions!

I said so from my heart. Truly.

It was not a "win" in debate that I was wanting.
I wanted to figure out what decison is best, along with good, thorough understandings and analysis.

I am satisified with this fruitful ending.

I just wish the dicussion could be smoother...

[grandsong]

PS:
I took a leap in logic when I wrote ...

2. animal is defined, in data.

It is the case of #7631 BECAUSE:
If this happens, the developer should orginally mean to write animal == person.pet.

He wouldn't compare two things if they did not both exist, would they?

I thought this is obvious so I didn't explicitly explain so.

That might have made you unable to connect this case with that issue quickly.

And as a result, you didn't realize it was exactly about "people who aren't intentionally using assignment and expect to declare something new, but by mistake"