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

Comments

Projects
None yet
@englishextra

englishextra commented Apr 13, 2012

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

This comment has been minimized.

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 Apr 14, 2012

@douglascrockford

This comment has been minimized.

douglascrockford commented Apr 14, 2012

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

@douglascrockford

This comment has been minimized.

douglascrockford commented Apr 14, 2012

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

This comment has been minimized.

Member

fat commented Apr 15, 2012

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

@kitcambridge

This comment has been minimized.

kitcambridge commented Apr 15, 2012

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

Zzzzing!

@sferik sferik referenced this issue Apr 15, 2012

Closed

Add semicolon #3069

@backspaces

This comment has been minimized.

backspaces commented Apr 15, 2012

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

@stephenhandley

This comment has been minimized.

@adrusi

This comment has been minimized.

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

This comment has been minimized.

coolaj86 commented Apr 15, 2012

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

This comment has been minimized.

zacstewart commented Apr 15, 2012

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

This comment has been minimized.

dubcanada commented Apr 15, 2012

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?

@davidk01

This comment has been minimized.

davidk01 commented Apr 15, 2012

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

@zacstewart

This comment has been minimized.

zacstewart commented Apr 15, 2012

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

s3u commented Apr 15, 2012

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

@devinrhode2

This comment has been minimized.

devinrhode2 commented Apr 15, 2012

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

This comment has been minimized.

markjreed commented Apr 15, 2012

@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

This comment has been minimized.

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

This comment has been minimized.

frewsxcv commented Apr 15, 2012

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

@englishextra

This comment has been minimized.

englishextra commented Apr 15, 2012

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

This comment has been minimized.

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

This comment has been minimized.

devinrhode2 commented Apr 15, 2012

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

This comment has been minimized.

tbranyen commented Apr 15, 2012

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

This comment has been minimized.

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

This comment has been minimized.

chuckbergeron commented Apr 15, 2012

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

This comment has been minimized.

ajacksified commented Apr 15, 2012

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

This comment has been minimized.

michaelficarra commented Apr 15, 2012

@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

This comment has been minimized.

NARKOZ commented Jul 30, 2012

@wamatt

This comment has been minimized.

wamatt commented Sep 9, 2012

@NARKOZ epic win on an already hilarious thread :D

@tomasdev

This comment has been minimized.

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

This comment has been minimized.

flavius commented Nov 24, 2012

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

@marcooliveira

This comment has been minimized.

marcooliveira commented Dec 6, 2012

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

@blpiltin

This comment has been minimized.

blpiltin commented Dec 12, 2012

"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

This comment has been minimized.

ghost commented Dec 12, 2012

You should always use semicolons with curly-brace languages.

@wprl

This comment has been minimized.

wprl 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
@enrmarc

This comment has been minimized.

enrmarc commented May 14, 2013

Well, I just want to be part of this;

@backspaces

This comment has been minimized.

backspaces commented May 14, 2013

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

This comment has been minimized.

greg0ire commented May 25, 2013

@vpatryshev

This comment has been minimized.

vpatryshev commented Jun 26, 2013

"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

This comment has been minimized.

chrisharrison commented Jun 27, 2013

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
.

@activars

This comment has been minimized.

activars commented Jun 29, 2013

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

semicolons

@MathRobin

This comment has been minimized.

MathRobin commented Jul 23, 2013

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

This comment has been minimized.

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

Merge pull request #15 from kevinvincent/master
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.