Skip to content
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

fix(compiler): return handler value for event modifiers instead of un… #7704

Merged
merged 4 commits into from
Mar 8, 2018
Merged

fix(compiler): return handler value for event modifiers instead of un… #7704

merged 4 commits into from
Mar 8, 2018

Conversation

STUkh
Copy link
Contributor

@STUkh STUkh commented Feb 23, 2018

fix(compiler): return handler value for event modifiers instead of undefined

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:

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:
Without modifiers compiler return original vm function, so we can expect to handle returned value (as example - handle it in directive)

on: {
    click:  _vm.test($event)
}

With modifier it always return "undefined" because of missing return value

on: {
    click: function($event) {
        $event.preventDefault()
        _vm.test($event)
    }
}

This PR add correct return value if modifier wrapper is used

on: {
    click: function($event) {
        $event.preventDefault()
        return _vm.test($event)
    }
}

@jacekkarczmarczyk
Copy link

How will it behave with the following example? https://codepen.io/anon/pen/ddwWKW

The handler code is alert('x'); alert('y'), so it will be compiled to return alert('x'); alert('y') and the second alert won't be shown, am I right?

}
return `function($event){${code}${handlerCode}}`
return `function($event){${code}return ${handlerCode}}`
Copy link
Member

@Kingwl Kingwl Feb 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe handlerCode should be wrap in a IIFE

@STUkh
Copy link
Contributor Author

STUkh commented Feb 28, 2018

Agree with @jacekkarczmarczyk.
@posva , @Kingwl : Now return added only for single handler expressions. And result all the same with and without modifiers
@click="test" =>click: _vm.test
@click="test($event)" => click: function($event) { return _vm.test($event) }
@click="test($event)**;**" => click: function($event) { return _vm.test($event) }
@click="test1($event);test2($event)" =>

click: function($event) {  
    _vm.test1($event)
    _vm.test2($event)
}

And the same behavior with modifiers

@jacekkarczmarczyk
Copy link

What about @click="alert(';')"?
split(';') doesnt' seem to be the good solution

@posva
Copy link
Member

posva commented Feb 28, 2018

Maybe we should return only when the provided expression is a function: <div @click="doSomething"> but not when it is a statement or expression: <div @click="doSomething()"> It always possible to wrap things when you need to pass parameters:

<button @click=" e => doSomething('a parameter', e)">Click me</button>

@STUkh
Copy link
Contributor Author

STUkh commented Feb 28, 2018

@posva you're totally right. But there is a problem when we use modifiers. They wraps handler and we can't get result.

For now i'm stuck at parsing handler to check is multiple expressions inside of it
/\)(?:\s*);(?:\s*)(?!.*'|")/
http://take.ms/TcERo
If it contains single expression - add return; If not - leave handler as is.
A will be appreciate if someone help to check it correctly.
@jacekkarczmarczyk, @posva, @Kingwl maybe you could help? :)

@Kingwl
Copy link
Member

Kingwl commented Mar 1, 2018

thanks for your pr
please add more tests for the case

For now i'm stuck at parsing handler to check is multiple expressions inside of it
/)(?:\s*);(?:\s*)(?!.*'|")/

IMO parse expression with regex or other string operation is not a good chooice
we need to reconsider this feature
ping @yyx990803

@jacekkarczmarczyk
Copy link

jacekkarczmarczyk commented Mar 1, 2018

But there is a problem when we use modifiers. They wraps handler and we can't get result.

I didn't dig deep into the code so I'm not sure what exactly you mean by it, but maybe there's an easy way to implement something like handler.rawValue to get access to non-wrapped handler?

@Kingwl
Copy link
Member

Kingwl commented Mar 1, 2018

implement something like handler.rawValue to get access to non-wrapped handler?

we have not instanced the rawValue yet, the modifiers generated code has mixin with handler.
instanced the handler.rawValue will lead to runtime code size increases

Maybe we can implement it in a limited way as @posva said

@STUkh
Copy link
Contributor Author

STUkh commented Mar 1, 2018

@posva, @jacekkarczmarczyk, @Kingwl, Morning. I've re-implemented exactly as @posva proposed before.

Now we expected that
@click="test" => click: _vm.test
@click.prevent="test" =>

click: function($event) {
    $event.preventDefault()
    return _vm.test($event)
}

@click.prevent="test1();test2()" =>

click: function($event) {
    $event.preventDefault()
    _vm.test1()
    _vm.test2()
}

@@ -119,7 +119,7 @@ function genHandler (
code += genModifierCode
}
const handlerCode = isMethodPath
? handler.value + '($event)'
? `return ${handler.value}($event)`
: isFunctionExpression
? `(${handler.value})($event)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need to return here as well and adapt other test cases

@STUkh
Copy link
Contributor Author

STUkh commented Mar 1, 2018

@posva checked this case:
@click.prevent="(e) => test1(e)" compiled to:

click: function($event) {
                $event.preventDefault()
                ;(function(e) {
                  return _vm.test1(e)
                })($event)
              }

@STUkh
Copy link
Contributor Author

STUkh commented Mar 1, 2018

@posva pushed change. Now ^ example returns

click: function($event) {
                $event.preventDefault()
                return (function(e) {
                  return _vm.test1(e)
                })($event)
              }

@Kingwl
Copy link
Member

Kingwl commented Mar 2, 2018

add some other test case for method path and function expression please

@STUkh
Copy link
Contributor Author

STUkh commented Mar 3, 2018

@Kingwl there are 7 function expression tests was there. I've added more cases with methods and so on.

@@ -79,6 +79,9 @@ function genHandler (
const isMethodPath = simplePathRE.test(handler.value)
const isFunctionExpression = fnExpRE.test(handler.value)

const handlersArr = handler.value.split(';')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split(';') is not a good chooice

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

Successfully merging this pull request may close these issues.

6 participants