lint-trap
lint-trap copied to clipboard
uber-archive
Rule to ban `.bind()` and force `var self = this`
Joshua mentioned that we had multiple production bugs that didnt get caught in code review that came down to a misunderstanding of this.
If we can author an eslint rule to enforce consistency around this that would be cool.
cc @jcorbin @malandrew
I wholeheartedly disagree on this one. How did people use bind that caused problems in production, but not in development or were not caught by tests? I don't see how bind can be any more problematic than call or apply. When bind is misused, it breaks code pretty spectacularly. Plus, banning bind, makes partial application a pain.
Personally, I would ban this before banning bind, since that is the root of the issue here.
bind is a performance issue. V8 deoptimizes on encountering bind.
On Thu, Oct 23, 2014 at 11:32 AM, Andrew de Andrade < [email protected]> wrote:
I wholeheartedly disagree on this one. How did people use bind that caused problems in production, but not in development or were not caught by tests? I don't see how bind can be any more problematic than call or apply. When bind is misused, it breaks code pretty spectacularly. Plus, banning bind, makes partial application a pain.
Personally, I would ban this before banning bind, since that is the root of the issue here.
— Reply to this email directly or view it on GitHub https://github.com/uber/lint-trap/issues/10#issuecomment-60286739.
bind is slower, but closuring with var self = this; requires the function definition to be inlined. With bind you can call that code from anywhere, which gives you greater flexibility in code organization.
I'm okay with banning bind only if this goes along with it. If we go that route, I'll use a special purpose function just for partial application (TBH, bind probably never should have been overloaded to both bind context and perform partial application)
If people really want bind and understand it, they can use an emulated bind which just returns a function that calls apply. In this case, anyone explicitly using emulated bind probably knows what they are doing.
@malandrew bind is not problamatic.
What's problematic is deeply nested code and forgetting to call .bind() at every layer of nesting.
If you do one var self = this; at the top of a 200 line method then you know you are good.
Basically as part of code review, spotting a missing var self = this; is easier then spotting a missing .bind(this).
@malandrew
Now that I think about it, it's not banning bind that is needed, it is banning this ONLY inside closures that is needed, which automatically forces the var self = this; pattern.
Definitely not a fan of banning bind. If we ban bind we should never allow this. to be used at all and Always require var self = this; at the top of every function. Otherwise you run into the bugs where people do self.something() and they forget to define self at the top of the method. bind is actually less of a mental overhead than var self = this; and seeing if it is available for most people in my limited sampling.
