heroku / heroku-connect-plugin

CLI plugin for Heroku Connect (experimental)
Apache License 2.0
23 stars 8 forks source link

use var instead of let, for more familiar semantics #19

Closed gulopine closed 8 years ago

gulopine commented 8 years ago

Functionality, none of these places exhibit different behavior between var and let, but it's better to be in a habit of using semantics that are more familiar to us, either from a Python background or a more traditional JavaScript background. let is geared more toward a C++/Java background, and is likely to get us into trouble if we're not particularly careful with it.

hburrows commented 8 years ago

let (and const) is considered more modern and it's use is generally encouraged over var. let is scoped to the nearest enclosing block where var is always hoisted to function scope. So some of the conversions to var inside of nested blocks could actually be wrong unless you really know that the declared variable hasn't already been used. Airbnb's style guide (https://github.com/airbnb/javascript#references) is widely referenced and they discourage the use of 'var. Just my 2 cents.

gulopine commented 8 years ago

All of our uses of let/var are currently either module-level or function-level already, so the semantics don't change during the course of this particular PR. I verified each one as I put this together.

I did notice that let seems to be preferred in general. I hadn't seen the Airbnb style guide, but the Heroku docs on plugin development use let extensively, though they do so without any explanation. I had originally used let in accordance with what seemed to be modern practice.

However, the scoping rules of let are quite different from what many of us on the team are used to, either coming from Python or from more traditional JavaScript. As easy as it is to use var in the wrong place, it's also quite easy to use let in the wrong place, and have a variable mysteriously vanish at the end of an if block and not understand why. But since we're likely to be more familiar with how var works, it seems less likely that we'll make mistakes with it.

And to be fair, the Airbnb styleguide, offers absolutely no rationale for the discouragement of var. It describe the difference between the two, but offers no insight as to why let would be preferred when you're not in a rather specific situation. As far as I can tell, even the two examples they give in that exact rule are behaviorally equivalent.

So I'm erring on the side of caution at this point, as it seems easier to avoid shooting ourselves in the foot with var than let.

hburrows commented 8 years ago

I disagree with "it seems easier to avoid shooting ourselves in the foot with var than let". I think it's completely the opposite. If you're using a good linter (like eslint) you'll immediately know if you're referencing a variable that has gone out of scope. However, eslint can't provide the semantic analysis to know if you're using var correctly because of its hoisting behavior. For example, I can declare var as the last statement of a function and reference it in the first statement of the same function without error. let has normal block level semantics which modern high-level languages like C++/Java/C#/Go, etc have. I think the reference point should be "what would a competent Javascript programmer expect/understand" verses "what would someone coming from Python expect/understand". This is Javascript after all. It should be written using modern, idiomatic Javascript and not Javascript using Python semantics. You're using generators all over the place which are very modern Javascript constructs. Why not embrace let and const (both of which have proper block level scoping) which directly address past problems with the language?

gulopine commented 8 years ago

Honestly, the answer to "why not embrace let?" is simple: I've already been bitten by it a few times. I used let at the function level, then used it again inside an if block, without remembering that I had already declared it at the function level. The if block changed its value, but once that block ended, the value reverted, even though I didn't want it to. A linter wouldn't have caught that, because I did in fact have it declared at the function scope, and it could have no way of knowing what my intention was. And, to be fair, that's not actually because of a Python background. That's from a JavaScript background, with 15+ years of knowing how var works, and forgetting that let is quite different.

I'm not opposed to let, but I don't see any justification for its use across the board. There may well be situations like those in the MDN documentation where we need to use those semantics, and in those cases, I'll gladly use it. But in cases where there's no semantic difference between var and let, I see no reason to summarily abandon idiomatic JavaScript.

When it comes to promises and generators, their advantages are plain to see, and are there, regardless of context. But when let only provides an advantage in a very limited use case, I find it hard to justify dropping everything in favor of it. It just seems like the ages-old JavaScript mentality of jumping on the newest thing because it's new. Call me a little gun-shy after shooting myself in the foot with it.

That said, at the outset of all this, I had a goal of trying to be consistent with not only other JavaScript developers, but other toolbelt plugins as well. Heroku usage is let across the board, and if I hadn't run into problems with it, I probably wouldn't think twice about doing so. But I also feel that it's pragmatic to make sure that we can all actually work on this stuff. If everybody wants to use let, I'm happy to do so. I may not see any benefit to it, but I also don't see any problem with it, as long as we all know how to use it. I believe we do all know how to use var, and I just don't know if that's true of let yet.

gulopine commented 8 years ago

Alright, further discussion has landed us on let. I'm closing this and I'll open a new one to go let all the way.

hburrows commented 8 years ago

Seems like this is becoming a pissing contest which is not my intention at all. I'm not sympathetic to your example of getting bitten by using let and declaring it function scope and block scope and loosing the value when it goes out of scope -- that's exactly how it's suppose to work and the beauty of block-level semantics. That's programmer error and something that was probably easily caught and debugged. Very few programmers (experienced Javascript programmers included) comprehend Javascript's bizarre variable and function hoisting behavior and get in trouble unknowingly because of it. You can characterize let and const as the shiny new thing but I believe they are a well thought-out attempt to modernize and evolve the language and correct long-standing issues and limitations with the language.

When I saw this PR and all the unnecessary changes of let to var I couldn't help but react because 1) it's an unnecessary change, 2) it's moving backwards instead of forwards, 3) contrary to Heroku's own examples in the plugin development docs (as you admit), and 4) contrary, IMHO, to what would be considered modern JS best practices (we are Heroku and we do want to be modern and progressive don't we??)

That said, I still regularly use var when I'm declaring something function scope (habit I guess). I might go through code and change a bunch of vars to lets, but I would never convert a bunch of lets to vars. That feels like moving forward looking through the rearview mirror.

If you really feel this is making the code better, more maintainable, and more robust then by all means merge away.