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

Autobinding handlers syntax (#109) #131

Closed

Conversation

nippur72
Copy link
Contributor

Discussed in #109, this PR extends the syntax for 'on' event handlers allowing a simple function handler to be bound to this without creating an injected function.

Example:

<div onKeyDown="this.handleKeyDown"></div>

generates

'onKeyDown': this.handleKeyDown.bind(this)

Notice how this.handleKeyDown is out of the { }. The .bind(this) is appended only if the handler starts with this..

This syntax can be seen as a shortcut for onKeyDown="()=>this.handleKeyDown() with the advantage that no embedded function is created.

So to sum up, the whole syntax for event handlers would be:

<div onKeyDown="{this.handleKeyDown.bind(this)}"></div>  <!-- old way --> 
<div onKeyDown="this.handleKeyDown"></div>               <!-- new syntax --> 
<div onKeyDown="()=>this.handleKeyDown()"></div>         <!-- with lambda function --> 

@yotambarzilay
Copy link

This is only relevant for es6 class syntax. React.createClass does autobind all functions, and in the case on this PR, will leads to 2 bindings.

@nippur72
Copy link
Contributor Author

@yotambarzilay you are right, I had not realized React.createClass and React.Component behave differently.

This shocks me because it means that your rt-template might not work as expect depending if your are using it from ES6 or not...

Also: if you use the ()=> lambda syntax along with React.createClass actually there are two bindings, right? (the one made by the lambda and the one made by React.createClass).

Regarding this PR, what about if we add a command line switch e.g. --autobind ?

@nippur72
Copy link
Contributor Author

Ok, I've made available this with the command line switch --autobind (false by default for backward compatibility)

I think that makes sense as more and more people will move to ES6 and perhaps oneday React.createClass might be deprecated:

Our eventual goal is for ES6 classes to replace React.createClass completely, but until we have a replacement for current mixin use cases and support for class property initializers in the language, we don't plan to deprecate React.createClass.

@nippur72
Copy link
Contributor Author

nippur72 commented Jul 8, 2016

An additional remark: we should investigate the impact of bind() over performances and choose the best model for the emitter. See this blog post entry: Don't Use Bind When Passing Props.

idok pushed a commit that referenced this pull request Jan 9, 2017
@idok
Copy link
Contributor

idok commented Jan 9, 2017

accepted

@idok idok closed this Jan 9, 2017
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.

3 participants