Skip to content

Do NOT throw warning when assigment in v-if happens #10184

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

Closed
grandsong opened this issue Jun 23, 2019 · 22 comments
Closed

Do NOT throw warning when assigment in v-if happens #10184

grandsong opened this issue Jun 23, 2019 · 22 comments

Comments

@grandsong
Copy link

grandsong commented Jun 23, 2019

What problem does this feature solve?

[UPDATE] I borrowed the idea "Optional Binding" from Swift which is famous. Unfortunately, this makes the topic astray... Please be noted that the real core here is "assigment in v-if".

First off, what is "Optional Binding"?

Sounds fashionable. Yes, it is, in Swift.

In case you don't quite recall, below is the statement of this useful feature of Swift:

if let constantName = someOptional {
    statements (using that constantName)
}

Which is also called "if let" statement.

And the doc is here: https://docs.swift.org/swift-book/LanguageGuide/TheBasics.html#ID333

So, what's its counterpart in Vue?

For example:

I have data of a list of trainees and a dictionary of training records.

For some reason, the two are separated.

I have to check if a trainee has records, and if so, show details of the records.

.trainee-cards
    .trainee-card(v-for="trainee in trainees" key="trainee.id")
        .trainee-name {{trainee.name}}
        section.training-records
            .training-records-details(v-if="training-records-by-trainee[trainee.id]")
                .count Count: {{training-records-by-trainee[trainee.id].count}}
                .last-date Last trained on {{training-records-by-trainee[trainee.id].recent-records[training-records-by-trainee[trainee.id].recent-records.length - 1].date}}
                .recent-records
                    .record(v-for="record in training-records-by-trainee[trainee.id].recent-records")
                        ... (a line chart of last 5 trainings) ...
            .no-records(else) No records yet.

You can see training-records-by-trainee[trainee.id] is used many times and the codes are terribly long and messy.

This example is a bit extreme, but you can get the idea.

The good news is: we can assign a local variale in v-if!

...
            .training-records-details(v-if="record-data = training-records-by-trainee[trainee.id]")
                .count Count: {{record-data.count}}
                .last-date Last trained on {{record-data.recent-records[record-data.recent-records.length - 1].date}}
                .recent-records
                    .record(for="record in record-data.recent-records")
                        ... (a line chart of last 5 trainings) ...

This makes life much easier.

Alternatives?

Yes, I know, we can always use a component to handle record-data.

But that would be overkill, unless the component can be reused.

Another way to ease the pain would be baking data beforehand.

For example,

    data: function(){
        trainees.forEach((trainee)=>{
            trainee.record-data = training-records-by-trainee[trainee.id]
        });
        return {
            trainees
        };

Then we can write trainee.record-data.

Neeter than training-records-by-trainee[trainee.id], but not as neet as "optional binding".

So, what's the problem?

This "optional binding" already works well and is as helpful just as in Swift.

Then, here's the bad news: it invites such a warning (actually, a red error):

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

This is an eyesore.

What does the proposed API look like?

Just make vue recognize optional binding and thus do not warn about the local variable, which IS "defined on the instance".

Possible controversy

Some people may dislike the idea of "allowing" or even "encouraging" assignment in if condition.

An example is issue #7631: "warn when assignment in v-if in development mode"

As @posva said, "this is a js common pitfall for beginners" to mistakenly write "=" when they actually want "==".

And they usually would lean the lesson in hard way, having paid much for such mistakes.

I was one of them, of course. So I know how natural it is to have a reflexive aversion to "optional binding" when learning Swift.

But after a while, I realized we should not object to a very good thing only because of some other mistake, which is not its fault.

In pure js, even today, you can still make such a mistake like if(a = b){console.log(a);}, and you get a global window.a as an unexpected result.

I hope someday "strict mode" will forbid this noxious pitfall and optional binding will be introduced into JavaScript, just like "for let" which already is in ES5.

Anyway, in vue, there is no worry about mistakenly creating a global variale via assignment in v-if.

No matter why, vue has the feature "optional binding" already, partially but effectively!

It is just unknown to the public. It is a hidden gem.

And that warning makes developers feel uneasy, although it is not a result of intention of guarding "=" in "v-if" at all.

@posva
Copy link
Member

posva commented Jun 23, 2019

Creating variables in a v-if is a hack. Use https://github.com/posva/vue-local-scope if you need to create local variables

@posva posva closed this as completed Jun 23, 2019
@grandsong
Copy link
Author

grandsong commented Jun 24, 2019

Thanks for your quick response.

However, sadly, you closed this issue as quickly.

And I don't quite understand what you mean by "hack".

I presume you are saying it...

  • either not worth becoming an official feature of vue
  • or, a certain alternative without modifying vue already can satisfy the demand.

Let me discuss them.

--

Optional binding is essentially a sugar syntax in Swift, for a specific but very meaningful purpose.

Yes, people can always write something different and get similar results, but that does NOT mean the sugar syntax is unnecessary.

Swift officially added this sugar syntax as a feature, for very good reasons.

And I was suggesting the same thing to vue.


"Creating local variables" is not the core of the problem.

As I said, we have alternatives already.

In fact, I'm using "baking data beforehand" for now.

I can live with that. Developers can always live with such inconvenience.

But I am requesting a better life with vue.

Vue has evolved with several improvements which are not "indispensable" but ease a lot of pain.

A simple example is :name for v-bind:name.

I hope this good tendency continues.


I've learnt your vue-local-scope before. This is about scoped slots, which I seldom use.

If I use vue-local-scope alongside scoped slots, it would just be another overkill.

The core of issue is:

  • "Optional binding" already works in vue.

  • It is the neatest and most elegant way, beating all other alternatives.


Please kindly reevaluate this issue.

@Justineo
Copy link
Member

I'd rather use computed to create a better data structure for your case, with proper filter/map transformations on the original input. It'll look much cleaner than doing all the heavy work inside templates.

@grandsong
Copy link
Author

I'd rather use computed to create a better data structure for your case, with proper filter/map transformations on the original input. It'll look much cleaner than doing all the heavy work inside templates.

Please read my words more thoroughly.

There are reasons that sometimes I have data separated.

Yes, I can bake data beforehand. In the example, I use forEach and get trainee.record-data for each member of trainees.

This is how I do for now. I can live with that.
But if this was enough, I would not write such a long issue at all.

@posva
Copy link
Member

posva commented Jun 24, 2019

It's a hack as in using a language pitfall as a feature:

// this throws in strict mode
if (a = 3) doSomething()

should be

let a
if (a = 3) doSomething()

For the same reason, you should declare the variable if you set it in a v-if directive. Variables cannot be declared in the template

I hope this clears out things 🙂

@grandsong
Copy link
Author

grandsong commented Jun 24, 2019

Firstly, your code is not the same thing as optional binding.

The correct counterpart in JS is:

Either before ES6 without let:

(function(){
	var a = 3
	if (a) doSomething()
})()

Or with let:

if (true)){
	let a = 3
	if (a) doSomething()
}

Again, as I said, "optional binding" is essentially a "sugar syntax"

But it is an official feature in Swift, which spares a lot of chores.

Swift team decided to do so, instead of suggesting "well, you guys always have alternatives (like in many other old languages), so you should write long codes or make your own hacks (or borrow from others) and live with that!"


Secondly, it was not pure JS that I was concerning.

The vue has its own particular uses, and one of them makes "optional binding" very favorable.

Surprisingly, vue already allows "optional binding", no matter purposely or not.

I suspect not purposely. Maybe just like "Support destructuring assignment".
There is no explicit words in official doc saying you can do so, but vue does support this ES6 feature.

Anyway, it is already so close for me to enjoy "optional binding" in vue, if there would not be the annoying "warning".

@grandsong
Copy link
Author

grandsong commented Jun 25, 2019

Just now, when I prepared some data with forEach and map so that I won't need to calculate in templates, I got this:

[Vue warn]: Error in render: "TypeError: Converting circular structure to JSON"

The cause here is:

I have some paradigms and games. One paradigm has many games.
There are cases that a game needs to check some props of its paradigm.
So what I did just now was I forEach-ed every game and added game.paradigm_ = CC.paradigmz[game.pid], where CC.paradigmz is the dict of all paradigms and pid is the id of the paradigm owning that game.

Well, this makes circular structure between paradigms and games.
As I predicted, it should be OK, according to my business logic.

But I hadn't anticipate that vue automatically calls JSON.stringfiy on every member of data in the component.
So, calculating in templates (or in computed) IS required sometimes...

@Justineo
Copy link
Member

vue automatically calls JSON.stringfiy on every member of data in the component

That's not true: https://jsfiddle.net/Justineo/01ezbdyg/

@grandsong
Copy link
Author

grandsong commented Jun 25, 2019

vue automatically calls JSON.stringfiy on every member of data in the component

That's not true: https://jsfiddle.net/Justineo/01ezbdyg/

Thanks, @Justineo .

I don't know what was really going on, but I did encounter that error and my codes broke.

I am sure it was vue who called JSON.stringify. I replaced JSON.stringify with a custom one in which console.trace(). I got the caller of JSON.stringify, which was from js vm. I clicked the "link" and in Source Panel I saw a long line of codes containing things of my vue component, definitely generated by vue.

And after I undid game.paradigm_, everything became OK again.

@Justineo
Copy link
Member

Maybe because I used a vue component while your fiddle is not a component?

https://jsfiddle.net/Justineo/01ezbdyg/5/


And what you want is not an optional binding, instead, it's just an ordinary assignment expression, the “lexical binding” is not optional here.

As Vue evaluates the compiled template code with the component instance, the develop-time proxy will see any undeclared variable as accessing a property on the instance because of the scope chain. Vue will never know whether an undeclared variable means a data property or a new local variable. The only way you can differentiate them is by using a variable declaration like let a = 1. But put it into an if statement's conditional expression is not allowed in JavaScript and will produce a SyntaxError.

@grandsong
Copy link
Author

@Justineo
Thank you very much. I wrote my own fiddle about my quick suspect and proved it wrong. So I deleted my words of that suspect. I didn't expected you responded so fast and did a similar experiment.

Just now, I redid game.paradigm_ = CC.paradigmz[game.pid].
Strangely, this time there is no error. game.paradigm_ can be used well everywhere.
There should be somthing I missed. But sorry that I cannot recall.
Anyway, it is a happy ending, for now.

@grandsong
Copy link
Author

grandsong commented Jun 25, 2019

Sorry I didn't clarify that my intention was not for vue to apply "optional binding" exactly as Swift.
The two are similar, not the same.
Yes, "it's just an ordinary assignment expression".
I metioned "optional binding" in Swift because it is famous. In fact, it changed my attitude towards "assignment in if", which I had believed evil as a kind of mark of newbies.
What I want is just to remove the warning.


The warning doesn't help anyone.

It looks like an error, but the codes can run well despite it.

It was not caused by a rule which forbids "assignment in v-if".

There in no such a rule. And in issue #7631, @posva decided not add such a rule, which I totally agree.

It was caused as you explained: vue cannot recognize the assignment and thus the new variable.


But there's a fact that vue recognizes new variables in v-for.

vue even supports "destructuring assignment", which is a little secret that doc doesn't metion, waiting for people to discover.

So, I think it is very feasible for vue to recognize variables in v-if, and ignore them, instead of outputing those useless warnings.

@Justineo
Copy link
Member

So, I think it is very feasible for vue to recognize variables in v-if, and ignore them, instead of outputing those useless warnings.

No it's not. Vue doesn't know whether recordData is a missing property or a new local variable you want to create. v-for is a different story. You don't have such ambiguity in v-for, the identifier before in is always a new variable inside the scope.

@grandsong
Copy link
Author

Then it there ambiguity in v-if?
The identifier before = is not always a new variable inside the scope?

Do you mean of a case like this ...?

https://jsfiddle.net/69tq74xy/3/

Here, there's a pet in data while pet2s in v-if and sub nodes.

It works as expected.

Then, if I replace pet2s with pet, the pet in data will be reassigned.
As a result, the second line will become "The pet's name: Jerry". We have two "Jerry"s.

If I remove pet from data at all, and assign pet in v-if, pet can be used outside of v-if. We still get two "Jerry"s.

It works just like if(var pet = person.pet){...} ...(use pet)... in pure JS.
Since JS doesn't support if let, this is the status quo.
vue doesn't do anything special about this.

If a developer knows JS, and they should, they will understand this.

Anyway, it is surely aweful to write codes like the example. No one will purposely write so.

@Justineo
Copy link
Member

The identifier before = is not always a new variable inside the scope?

Currently v-for has a “micro-syntax” , which is <identifier> in <expression> so recognizing the variable name is rather trivial (note that Vue won't parse the <expression> part). 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.

@grandsong
Copy link
Author

grandsong commented Jun 25, 2019

You mean people may write codes like v-if="person.name && pet2 = person.pet"?

Yes, JS gives us a lot of freedom. Too much of it.
But this fact doesn't mean we should write weird, fancy codes..

I don't know whether Swift if let allows so, but even if it does, no serious Swift developer will write so.
That feature is for one purpose, and that one only.

This issue is similar.
The case: assigning one and only variale which will be used immediately under the v-if node.

My proposal here is:

  • If this case happens, recognize it.
  • If other messy, arbitrary cases happens, go on throwing warnings.

All sugar syntaxes target particilar cases. If a case is comon enough, it is worth a sugar syntax, or better, a official feature.
That's how Swfit is designed. I hope vue can follow its example.

@grandsong
Copy link
Author

Even in the source code of vue.js itself, there are uses like
else if ((slotScope = getAndRemoveAttr(el, 'slot-scope'))) { ... }

@grandsong grandsong changed the title Do NOT throw warning when optional binding happens Do NOT throw warning when assigment in v-if happens Jun 25, 2019
@grandsong
Copy link
Author

grandsong commented Jun 25, 2019

I've modified vue.js and successfully solved this issue.
Just 4 lines were added/modified.
I'll pull request soon, and privide a good fiddle demo for this "feature" as well.

@grandsong
Copy link
Author

grandsong commented Jun 25, 2019

Done.
Two demos:

  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.

grandsong added a commit to grandsong/vue that referenced this issue Jun 25, 2019
See vuejs#10184
Especially, the last comment of mine, in which I gives links of two demos.
@Justineo
Copy link
Member

Justineo commented Oct 12, 2019

https://github.com/tc39/proposal-Declarations-in-Conditionals

I suggest us keep an eye on this proposal and maybe this issue can be solved in JavaScript itself someday.

@grandsong
Copy link
Author

https://github.com/tc39/proposal-Declarations-in-Conditionals

I suggest us keep an eye on this proposal and maybe this issue can be solved in JavaScript itself someday.

Good to know.
Hope this proposal be accepted and implemented before 2020...

@Justineo
Copy link
Member

That's probably not gonna happen...it's only in Stage 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants