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

Unnecessary IIFEs pervasive in v4 implementation #139

Closed
dead-claudia opened this issue Jun 29, 2017 · 12 comments
Closed

Unnecessary IIFEs pervasive in v4 implementation #139

dead-claudia opened this issue Jun 29, 2017 · 12 comments
Assignees
Labels
enhancement potential improvement

Comments

@dead-claudia
Copy link

I was just looking at the source out of curiosity, and found a lot of seemingly unnecessary IIFEs in the collapse (1, 2) and tab (1, 2, 3, 4, 5, 6) implementations. Is there a particular reason for these, or should they be removed?

@thednp
Copy link
Owner

thednp commented Jun 29, 2017

The method is used because there is no cross-browser setImmediate method, and that fixes something reported over and over. I know it's not the most elegant method, but it's really hard to schedule those functions in that exact order, so, yea, it just works IE8+ no problem.

But I am open to any code suggestion you might have, consider backward compatibility as well.

@dead-claudia
Copy link
Author

@thednp I'm not referring to the setTimeout calls, but the plain IIFEs themselves, things like this:

(function () {
    // code...
})();

As for that bug fix, all you really did beyond adding the unnecessary IIFEs was move code into setTimeout callbacks and increasing their delay.

As for backwards compatibility, just inlining their bodies should have zero impact, even as far back as Netscape. Any differences would be obvious browser breakers, leaving almost all common sites with scripting unusable.

@thednp
Copy link
Owner

thednp commented Jun 29, 2017

@isiahmeadows as I said, I am open to any code suggestion, I really tried them all I know.

@dead-claudia
Copy link
Author

@thednp Check out #140. That should explain what I mean (it's also a nicely packaged PR).

@thednp
Copy link
Owner

thednp commented Jun 29, 2017

Wowow, wait a sec and check this first before committing.

Have you tested multiple clicks and it works perfect? I've spent about 2 days working to fix that issue, you think that would simplify and solve the issue for good? Also make sure the events are triggered when they should really trigger :)

@dead-claudia
Copy link
Author

dead-claudia commented Jun 29, 2017

It's equivalent to what's already there, and I have tested it to verify it does not break in Chrome.

@thednp
Copy link
Owner

thednp commented Jun 29, 2017

I am now testing your code in all browsers. It seems to work, but I don't know...

@dead-claudia
Copy link
Author

In case you're curious (I can tell you're not super familiar with the difference), here's the Wikipedia article on the idiom.

@thednp
Copy link
Owner

thednp commented Jun 29, 2017

OK. I remember now. FYI: I am not targeting G Chrome only, I am interested in IE8+ for BSN V3 and IE10+ for BSN V4. I also have a very old Safari for Windows.... My wild code there is meant to make it work in all the browsers consistently.

So, you know why those IIFEs are used? It's because the API on event.transitionEnd is poorly implemented in many browsers and it always messes up... sometimes things that need to happen when the animation ends, never happen, or the event is not even triggered.

So FYI, please take the time to test differences with all functionalities in regards to components custom events, browsers, consistency, etc ;)

@thednp
Copy link
Owner

thednp commented Jun 29, 2017

@chenull I hope you take a look at @isiahmeadows' commit as well. Thank you.

@dead-claudia
Copy link
Author

@thednp IIFE's are just a lanugage idiom, unique to JS the language, without respect to the environment. They do literally nothing apart from creating a new scope for var variables.

@thednp
Copy link
Owner

thednp commented Jul 2, 2017

@isiahmeadows I will certainly do a mega test with your suggestion as soon as I get some time. Thank you.

@thednp thednp closed this as completed Jul 2, 2017
@thednp thednp added the enhancement potential improvement label Jul 2, 2017
thednp added a commit that referenced this issue Jul 6, 2017
thednp added a commit that referenced this issue Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement potential improvement
Projects
None yet
Development

No branches or pull requests

2 participants