Improve JS style

[XhmikosR]

/CC @twbs/team

[cvrebert]

I would recommend excluding the minified files and npm-shrinkwrap.canonical.json so as to decrease the likelihood of merge conflicts.

[XhmikosR]

I don't mind rebasing for those only.

[XhmikosR]

@fat: I rebased the branch.

[fat]

cool

[fat]

which rule needs us to do that?

[XhmikosR]

Rebased again and addressed a couple of your comments @fat.

I didn't remove 44ca738 because it's easier to read IMO.

For 573aa5b I don't really mind, if you insist on keeping laxbreak I can revert that too, but personally I prefer it this way.

Let me know if I missed anything.

[fat]

in general things i like:

• adding correct spaces around function () {} keyword
• adding spaces in object literals ({ foo: bar })
• identifying redundant variable definitions

things i dont like:

• i dont like forcing === (to good of friends with @angus-c to allow that i think)
• i dont like moving all ternary's to if/else (maybe down the line, but for now not crazy about it)
• i dont like unfolding single line expressions (like functions, etc.)

[XhmikosR]

I can keep the if/else in one liners but I still believe it's cleaner this way; the minifier will output the same code anyway.

For === it's either this or not. And personally I'd rather have a simple enforcing option everywhere than mixed stuff.

[XhmikosR]

So to summarize, things we still disagree:

1. laxbreak which I don't really mind reverting but I still find it easier since we read from left to right
2. if/else when there's no assignment. The reason you probably don't like this is the curly brackets but this PR included brackets everywhere in the beginning. I can switch those to one liners, but I still prefer if/else than foo && bar; it's easier to spot the statement this way.

[fat]

yeah, im not sold on laxbreak and if/else

[fat]

for small expressions: e && e.stopPropagation() for example, i think it's fine and reads easy.

for longer statements, i could see changing these to if condtions. but i dont like having a hard rule around it.

[XhmikosR]

@cvrebert: can you check out this branch and update shrinkwrap? Otherwise Travis will fail since it will use JSHint 2.5.0.

[cvrebert]

@XhmikosR Done.

[XhmikosR]

Thanks. BTW, what do you think of that last patch? Personally, I think it makes sense and reduces duplication.

[fat]

&& on the next line looks so much better

[fat]

things i still want:

• multi line expressions should start with || or && operators.
• i actually prefer the ternaries in a lot of places you are removing them in favor of if statements. not crazy about that
• single line'd object literals shouldn't be unfolded. if they are small, they look better on a single line.

[fat]

self.tip().hasClass('in') 
  ? self.leave(self) 
  : self.enter(self)

[XhmikosR]

@fat: everyone needs to compromise a little bit. Sure, many changes are a matter of taste, but at least @cvrebert and I agree on these changes.

I removed laxbreak but I don't like the ternary operators compared to if statements. Readability isn't about the shortest possible way all the time; it's also about being explicit and making the code easier to understand, faster.

BTW, there's no enforcing option enabled for this. exp is disabled.

[XhmikosR]

Rebased and skipped more stuff. I still want to have these changes eventually, but for now this should be ok in order to merge it.

[fat]

do we need all these checks… who is extending the object prototype?

[XhmikosR]

In theory we do, in practice I doubt we'll ever have a problem. This branch was initially intended to make things stricter. Not just to make a tool happy, but to reduce the chances of making mistakes and having issues.

[fat]

http://javascriptweblog.wordpress.com/2011/02/07/truth-equality-and-javascript/

The avoidance strategy [of ==] bothers me because you can’t master a language until you know it inside out – and fear and evasion are the enemies of knowledge. Moreover pretending == does not exist will not let you off the hook when it comes to understanding coercion because in JavaScript coercion is everywhere! Its in conditional expressions (as we’ve just seen), its in array indexing, its in concatenation and more. What’s more coercion, when used safely, can be an instrument of concise, elegant and readable code. -- angus croll

In line with this, I'd rather we drop the hard rule of forcing ===

[XhmikosR]

I started this branch with the stuff I liked. Then I backed out many stuff in order to come to an agreement with @fat and I basically reverted half of the changes.

I don't see this getting anywhere, unfortunately.

[ludo237]

Amazing work @XhmikosR 👍

[cvrebert]

I cannot disagree more strongly with the pro-== camp. But I suspect no opinions are going to change on that issue.

[angus-c]

I worry when I see x == null blindly changed to x === undefined in a million places.
You're actually changing the logic.
The former means x === null || x === undefined. The latter means x === undefined.
The former is more through and more concise. No idea why you would prefer the latter.

I'd suggest you be a bit more thoughtful when making these blanket changes

[XhmikosR]

Nothing was done blindly.
On May 28, 2014 7:53 PM, "angus croll" notifications@github.com wrote:

I worry when I see x == null blindly changed to x === undefined in a
million places.
You're actually changing the logic.
The former means x === null || x === undefined. The latter means x ===
undefined.
The former is more through and more concise. No idea why you would prefer
the latter.

I'd suggest you be a bit more thoughtful when making these blanket changes

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13388#issuecomment-44434324
.

[fat]

@XhmikosR there's still some really good changes here, dont get too discouraged friend :)

still open to having a convo about this stuff. Also, I think it's good going forward to have smaller pr's. that way the conversation can be more focused, etc :)

[XhmikosR]

Well, if you weren't absent @fat for so long, the branch would have been way smaller :P

To summarize. As I have already told you privately, I totally respect the fact that you are the author of most of the JS in BS. I'm pretty sure you had, or even have, your reasons for your choices.

What I wanted with this PR is to make things more consistent and less error prone. Plus, I'm not the only one who likes the changes I made which is something that led me to go through with this after all, even though I couldn't talk to you back then.

Using JSHint isn't just something for fun; it really helps spotting issues like c93ac03 and 2a7f704.

Before I rebased the branch the other day, I used curly braces everywhere, I got rid of the one line statements and a couple more things. Due to the fact that we are all people and we do mistakes, I personally prefer being stricter, as long as there's no (obvious) downside.

I can break this PR in smaller patches; just tell me which stuff can go in, like now, and I'll rebase the branch.

[angus-c]

@XhmikosR I apologize then. I had assumed you were following JSHint's suggestions

[fat]

this seems inconsistent with our spacing around {, no?

[XhmikosR]

I haven't added spaces before [ and ]. I can do it in another patch if everybody else agrees with this.

[fat]

Well, if you weren't absent @fat for so long, the branch would have been way smaller

mm i would still prefer there to have been many little pr's to 1 big one. :)

just tell me which stuff can go in, like now

Everything in this branch is awesome to me, except the === rule which im still not sold on.

If you want to back out the === changes you can merge right away 👍

[XhmikosR]

@fat: note that currently === and !== are used in half of the places anyway.

[fat]

lgtm, thanks for suffering through this 😅

[XhmikosR]

@fat: keep in mind I still would like to have the changes I reverted eventually.