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

bootstrap-dropdown.js clearMenus() needs ; at the end #3057

Closed
englishextra opened this issue Apr 13, 2012 · 289 comments
Closed

bootstrap-dropdown.js clearMenus() needs ; at the end #3057

englishextra opened this issue Apr 13, 2012 · 289 comments
Labels

Comments

@englishextra
Copy link

bootstrap-dropdown.js when minified with JSMin::minify produces error in Firefox error console saying clearMenus()needs ;

on line:

  clearMenus()
  !isActive && $parent.toggleClass('open')

if in source code this is corrected -- no error in minified version

@fat
Copy link
Member

fat commented Apr 14, 2012

nope - that's a bug in jsmin. Probably should let @douglascrockford know about it though. thanks!

edit: The code had already been changed to an if when i suggested the jsmin issue be filed as a bug. Bootstrap and jsmin play very well together.

@fat fat closed this as completed Apr 14, 2012
@douglascrockford
Copy link

That is insanely stupid code. I am not going to dumb down JSMin for this case.

@douglascrockford
Copy link

TC39 is considering the use of ! as an infix operator. This code will break in the future. Fix it now. Learn to use semicolons properly. ! is not intended to be a statement separator. ; is.

@fat
Copy link
Member

fat commented Apr 15, 2012

i have learned to use them, that's why there isn't one present.

@ghost
Copy link

ghost commented Apr 15, 2012

i have learned to use them, that's why there isn't one present.

Zzzzing!

@sferik sferik mentioned this issue Apr 15, 2012
@backspaces
Copy link

Any language with syntax arguments is clearly broken, compilers deal with this. Dart, I guess.

@stephenhandley
Copy link

@adrusi
Copy link

adrusi commented Apr 15, 2012

if you really wanted to get rid of the semicolons (though I really don't see the point of that, is it really that bad that it's worth worrying about it?), ! ... && in this context an be replaced with ... ||.

@coolaj86
Copy link

coffeescript ftw?

otherwise, if you're doing real javascript, do it right?

p.s. (I'm not a coffeescripter yet, but it looks more and more like the right tool every day)

@zacstewart
Copy link

i have learned to use them, that's why there isn't one present.

Wow. I've read @fat's reasoning for not using semis, but when it comes to actual problems cropping up in the real world, why does "aesthetic" preference take precedence? Why write something like

!function( $ ){
...
}( jQuery )

just to avoid placing a semi a the end?

! is clearly not meant to do this job. It's a bool operator. Does the fact that the symbol looks prettier really matter?

I am well aware that you can hack your way around this and keep saying "nuh uh!" instead of admitting that it's ill conceived and improving, but seriously: making a snippy response like that just makes you look like an immature hipster smarting off to a battle worn professional. @douglascrockford is on the technical committee for fuck's sake.

@dubcanada
Copy link

This has nothing to do with being a hipster, and I have no idea why anyone seems to think it does. The simple fact is this code runs on ALL browsers without issue. Regardless if the fact that X version of javascript somewhere in the Y future will stop supporting it (maybe) does NOT give a reason for a javascript minifier to NOT correctly minify it.

Also if Crockford thinks this is insanely stupid code and he is on the technical committee then why is this insanely stupid code even possible?

@ghost
Copy link

ghost commented Apr 15, 2012

I know who @douglascrockford is but who is this @fat fellow?

@zacstewart
Copy link

Also if Crockford thinks this is insanely stupid code and he is on the technical committee then why is this insanely stupid code even possible?

Being on the technical committee in 2012 for a language initially created 16 years ago probably doesn't grant him authority to radically change things like that.

@mdo
Copy link
Member

mdo commented Apr 15, 2012

@zacstewart Jacob wasn't trying to snippy, he was responding directly to one person's aggressive remarks. Taken out of context I can understand how it might look that way, but side-by-side, there's no issue there.

All Jacob pointed out was that this is a bug in someone else's code and that guy comes in guns blazing instead of speaking calmly and objectively? I call bullshit on the whole situation. If semicolons aren't required, then we don't need to include them. It's as simple as that.

@jack9
Copy link

jack9 commented Apr 15, 2012

The simple fact is this code runs on ALL browsers without issue. Regardless if the fact that X version of javascript somewhere in the Y future will stop supporting it (maybe) does NOT give a reason for a javascript minifier to NOT correctly minify it.

Forcing unwanted paradigms has no business in code reviews.

Tools that reformat code can break code if the code is dependent on whitespace. Javascript is dependent on whitespace due to semicolon insertion. Javascript minification is not part of the language. So the code is correct for the author and they should not use a tool that breaks it.

I agree that the code runs and my first statement speaks to the freedom of an author to do as they wish. The freedom to code as they see fit. Those are compelling reason to NOT change it. However, the reality is that very few individuals will use the code unminified and the question of "correctness" falls to common convention as a matter of pragmatism. The middleground is to add a semicolon for general use of the code. Branch it and have a nice bootstrap-dropdown-minification_safe.js - There's nothing wrong with changing the code as you see fit to meet your needs.

Do not demand to change a tool because you want to use the tool in a way another author has explicitly said they will not support. That's hypocrisy. That's why people are getting upset.

@s3u
Copy link

s3u commented Apr 15, 2012

Learn to interop with existing toolset folks! This is a ridiculous debate.

@devinrhode2
Copy link

Semicolons ARE the recommended practice... not just from Crockford, but also in Google's JS style guide: http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Semicolons

@markjreed
Copy link

@eligrey - line break or not, Javascript never ends a statement if the next token is an infix or bracket operator. See http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Semicolons for some possibly surprising examples.

So if ! becomes an infix op, then newline + ! will no longer be equivalent to ; + !.

@tszming
Copy link

tszming commented Apr 15, 2012

nope - that's a bug in IE. Probably should let @billgates know about it though. thanks!

Do you agree with this?

While I agree that JSMin can be improved for this case, but you also :)

@frewsxcv
Copy link

If anyone is curious about the TC39 proposed syntax for the ! infix operator, here it is

@englishextra
Copy link
Author

Disagree with Mr. @fat approach: you distribute the code to developers and don't want to listen to good practices that are advised. I would have refactored the code when I had faced complaints from the users.

@jace
Copy link

jace commented Apr 15, 2012

Thanks to the stand-off on this issue, I have to maintain my own branch of Bootstrap with semicolons inserted so that it minifies gracefully. Keeping my repo in sync is not fun at all, so I'm deploying out-of-date versions with my apps.

Given how much pain making production use of Bootstrap was, it felt like a version 0.2, not 2.0.

@devinrhode2
Copy link

Clearly JSMIn isn't changing. That means Bootstrap can either add semicolons, or have people run into this issue again with JSMin. That's stupid, just use semi-colons.

Also, being a hugely popular library, people who have never developed a thing in their life are probably going to learn from bootstrap code - and emulate it. Then this newbie is screwed. Some brave soul decides to get his idea into the real world, finds bootstrap as it's the best thing out there for making beautiful apps, seeks to modify things, and picks up bad habits. Embracing bad habits is a dis-service to the whole JS community.

That's not cool.

Newbies are going to use bootstrap. They are going to learn from bootstrap.

@tbranyen
Copy link

Don't use JSMin with this project. Write documentation for newbies explaining why they shouldn't use JSMin. Don't tell the maintainer he has to do x or y for his own project. Fork it if you feel strongly enough to change the code.

@fat shouldn't change his code to work with JSMin and @douglascrockford shouldn't change his code so bootstrap can work with it. Just document why it doesn't work and move on?

@jyap808
Copy link

jyap808 commented Apr 15, 2012

Agreed, if you're making a general purpose tool like Twitter Bootstrap, make it as compliant as possible and use good practices.

Don't be a JavaScript hipster. Add semi-colons.

This is JavaScript. Relying on implicit insertion of semi-colons is stupid.

@chuckbergeron
Copy link

This minifies just fine via Rails' asset precompiling - I've never ran into an issue with it. IMHO, this is my issue with JavaScript as a language being much too flexible and forgiving.

@ajacksified
Copy link

tl;dr: use Coffeescript if you don't want semicolons.

Semicolonless Javascript is an ego-stroking attempt at rejecting standards for the sake of rejecting standards, not for the greater benefit of the community. While the need for hacks exists just to get around using semicolons, the practice does greater harm than good. "Use semicolons at the end of a statement" is a far simpler rule than "never use semicolons, except sometimes you have to use x hack, like prefixing with a !." All this for the benefit of an opinionated aesthetic? I propose that one might as well use Coffeescript instead, if the intent is prettier code by standards set as a lack of syntactic symbols.

Or, just write clean, standard (as defined by not just the specification, but as defined by the developer community) Javascript, if it is to be shared, used, and contributed to by the greater community.

@michaelficarra
Copy link

@douglascrockford: Regardless of whether you consider this usage of ASI a bug, it'd be ignorant not to acknowledge that there certainly is a bug in JSMin.

@NARKOZ
Copy link

NARKOZ commented Jul 30, 2012

@wamatt
Copy link

wamatt commented Sep 9, 2012

@NARKOZ epic win on an already hilarious thread :D

@tomasdev
Copy link

tomasdev commented Nov 6, 2012

I think this is all about @fat trying to be hipster. You know... step over all style guides currently available at the moment. YAY!

Personally, I like, and I think the only correct answer is the first @douglascrockford comment. There's nothing else to add.

@flavius
Copy link

flavius commented Nov 24, 2012

For the record, this was worth a talk: http://vimeo.com/53218578

@marcooliveira
Copy link

Just want to be part of web history. Haha... Hilarious.

@blpiltin
Copy link

"For verily I say unto you, Till heaven and earth pass, one jot or one tittle shall in no wise pass from the law, till all be fulfilled." -Matthew 5:18

@ghost
Copy link

ghost commented Dec 12, 2012

You should always use semicolons with curly-brace languages.

@ghost
Copy link

ghost commented Feb 13, 2013

        You don't have to put periods at the end.
                                At the end of sentences.
 But it can make them easier to read.

 On the other hand 
    there's no real reason not to
        just do whichever you want

@ghost
Copy link

ghost commented May 14, 2013

Well, I just want to be part of this;

@backspaces
Copy link

Yes, it certainly has become legendary;

On Tue, May 14, 2013 at 4:08 AM, Enrique notifications@github.com wrote:

Well, I just want to be part of this;


Reply to this email directly or view it on GitHubhttps://github.com//issues/3057#issuecomment-17867291
.

@greg0ire
Copy link

Relevant: We use semicolons everyday

@vpatryshev
Copy link

"The only true law is that which leads to freedom" (R.Bach)

Any style question starting with "why don't you..." has an easy answer: "because this is my style".

If you like semis, you use them; if you don't, you don't use them. What can be easier?

@chrisharrison
Copy link

Vlad,

That would be perfectly true if this was just a style issue. And I'd be
100% behind you if that was the case. Unfortunately this is about much more
than styling. JavaScript needs the semicolons to function logically.
On 27 Jun 2013 06:02, "Vlad Patryshev" notifications@github.com wrote:

"The only true law is that which leads to freedom" (R.Bach)

Any style question starting with "why don't you..." has an easy answer:
"because this is my style".

If you like semis, you use them; if you don't, you don't use them. What
can be easier?


Reply to this email directly or view it on GitHubhttps://github.com//issues/3057#issuecomment-20083328
.

@j1n6
Copy link

j1n6 commented Jun 29, 2013

There might be something else we want to focus on, leave the semicolons alone.

semicolons

@MathRobin
Copy link

Please use semicolons. Readbility is important. Maybe, you, @fat, know how and when using semicolon but it's not the case for all. Rookies, kiddies and even experts could need semicolon to read the code. So, please, add it for readability...

@torifat
Copy link

torifat commented Feb 24, 2014

Why don't we just get rid of indentations too??? It's really OPTIONAL. And, also MINIMALISTIC.

All the current IDE's are doing it wrong. Either they should give diff background colors to the different scopes or show them AII (AUTO INDENT INSERTED).

@twbs twbs locked and limited conversation to collaborators Jun 9, 2014
mdix referenced this issue in fbrandel/ParisHilton.js Aug 14, 2014
Minified further - Removed totally unnecessary semicolon
automatic-frog added a commit to osp/osp.tools.visualculture that referenced this issue Jan 1, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.