twbs / bootstrap

The most popular HTML, CSS, and JavaScript framework for developing responsive, mobile first projects on the web.
https://getbootstrap.com
MIT License
170.54k stars 78.85k forks source link

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

Closed englishextra closed 12 years ago

englishextra commented 12 years ago

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 commented 12 years ago

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.

douglascrockford commented 12 years ago

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

douglascrockford commented 12 years ago

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 commented 12 years ago

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

ghost commented 12 years ago

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

Zzzzing!

backspaces commented 12 years ago

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

stephenhandley commented 12 years ago

http://www.youtube.com/watch?v=rrPosTPSXxw

adrusi commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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

zacstewart commented 12 years ago

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 commented 12 years ago

@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 commented 12 years ago

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 commented 12 years ago

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

devinrhode2 commented 12 years ago

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 commented 12 years ago

@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 commented 12 years ago

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 commented 12 years ago

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

englishextra commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

@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.

aroc commented 12 years ago

At the end of the day, the repo maintainers can do whatever they please. We're not paying to use Bootstrap. They can include ASCII fingers all over the place if it makes them happy. If we don't like it, we can use something else or fork it. That said, I still think this discussion is valuable (minus the aggression), as these sorts of conversations get people thinking more deeply about code standards and how they fit into our everyday work as developers.

stephenhandley commented 12 years ago

+@ajacksified +@shama

jrz commented 12 years ago

Even though a semicolon might be 'better'. The syntax is correct, and jsmin should NEVER change or break working code. Period. so either stop using jsmin, or start fixing jsmin.

courthead commented 12 years ago

@ChadMoran I believe he's talking about putting the ! in front of that code.

jonashaag commented 12 years ago

@ChadMoran I think the point is function(){...}(); vs !function(){...}()

englishextra commented 12 years ago

/* Title: Immediate functions Description: syntax that enables function execution as soon as it is defined */

(function () { console.log('watch out!'); }());

//alternative with less parentheses !function () { console.log('watch out!'); }();

// reference // http://www.jspatterns.com/ // http://shop.oreilly.com/product/9780596806767.do?sortby=publicationDate

teiman commented 12 years ago

The point of using C syntax is to have ";" and "{" "}" to make sysntax and flow intend obvious to other programmers. This code is not obvious to other programmers (its not to me) so fix it by adding ";" on the end of expressions. Thanks.

istvan-antal commented 12 years ago

I don't think talking this much about a semicolon is worth it.

Does refusing to use a semicolon make the code go faster? NO Does it make the code harder to read and understand? YES Will it break in the future? Probably Does it have the bigger potential for future bugs? YES

Semicolon insertion was a mistake in JavaScript, along with eval, with and function scoping. People should be avoiding these instead of abusing them.

Semicolons are better for readability: if I see a diff, I know exactly where the line ends.

pyalot commented 12 years ago

I take it you really read ecmascript262 (and I assume you did because you use it as an excuse for the bad practice of omitting semicolons) then you should realize that JS grammar is ambiguous. It's particularly ambiguous where it's about what constitutes a statement if you omit a semicolon. Browsers implement heuristics when trying to parse it (a fancy word for guessing) and as a result don't always agree between each other. Because humans aren't very good at parsing a context-sensitive grammar and executing an approximate state-machine and if/else decision tree to figure out if something is a statement or not, we punny humans tend to do it wrong every once in a while, even if we're extremely well versed and hold the entire JS grammar in our heads (which would be a considerable feat).

Fortunately, there's a "fix" for this lamentable human condition. The fix is just to insert semicolons, even though technically it's not required in all cases. But I promise you, the time wasted writing out that semicolon is more than compensated by the time you will not spend hunting down heisenbugs due to browsers differing understanding of statements, the time not spent trying to make your JS code compatible to all JS-manglers (like JSmin, closure compiler etc.), the time not spent arguing in favor of an outdated and obviously bad practice with random people on the internet AND the time not spent fixing all your code when a browser implements a newer revision of JS.

shawwn commented 12 years ago

Hello everyone,

I've forked JSMin and implemented the desired behavior: https://github.com/shawnpresser/JSMin

It scans for "newline [whitespace] exclamation" and replaces the newline with a semicolon.

I hope this proves useful to someone (perhaps to the authors of Bootstrap). It was a fun project.

jace commented 12 years ago

@shawnpresser This means deploying your version of JSMin on all my systems, plus ensuring your version is in sync with upstream JSMin.

Why can't we instead just replace all \n! with \n;! in Bootstrap? It's easier and doesn't affect production environments.

jarinudom commented 12 years ago

Writing JavaScript without semicolons is like doing all of your HTML in quirks mode, with improper nesting, unclosed

  • tags, and unquoted attribute values.

    Just as HTML was designed so that a webpage would still probably look ok even with a sloppy idiot writing markup in HotDog, JavaScript was designed so that people writing code in Notepad would still probably get something workable if they forgot a semicolon here and there.

    Doesn't make it the "right" way to do it.

  • pyalot commented 12 years ago

    @shawnpresser that's wonderful, can you please implement it as well for the YUI compressor, googles closure compiler, dean edwards packer and microsoft ajax minifier?

    damien-m commented 12 years ago

    Having experience using Bootstrap 1.4 on a large JS application, I can tell you that Bootstrap (1.4 at least) falls apart when uglified.

    manojlds commented 12 years ago

    Any fool can write code that a computer can understand. Good programmers write code that humans can understand. ~Martin Fowler

    jlnr commented 12 years ago

    Couldn't @douglascrockford's JSMin just output an explanatory fatal error in this case, for the reason outlined in the third reply here? All programs that pass JSMin would still be syntactically intact. And JavaScript non-wizards (like me) would not sit there with broken code produced from two parts which we thought we could rely on.

    (And a big thanks to unknown for deleting all the meme/boobs.gif crap replies!)

    shawwn commented 12 years ago

    @pyalot I could try.

    mchampanis commented 12 years ago

    https://github.com/twitter/bootstrap/issues/401

    "fat: The reason it was closed is because semicolons were added to the end of lines in 1.4. In 2.0 we removed them again when we introduced the downloader as it safely concats and minifies these files for you. I'm adding semicolons to the end of files in 2.0.1 - which will likely be released tomorrow, to support this mobile oddity."

    cvn commented 12 years ago

    “Be liberal in what you accept, and conservative in what you send.”

    lcdt commented 12 years ago

    @shawnpresser Your fork does not minify this example correctly: if (confirm('Are you sure?') && !false) { alert('ok'); }

    shawwn commented 12 years ago

    @lcdt fixed; thanks.

    madrobby commented 12 years ago

    tl;dr Someone finds bug in JSMin, people that use Bootstrap (for free) make demands and lecture the authors about the one true way to write JavaScript.