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

Remove event aliases from JavaScript #12761

Merged
merged 1 commit into from
Feb 21, 2014
Merged

Conversation

hnrch02
Copy link
Collaborator

@hnrch02 hnrch02 commented Feb 16, 2014

Makes life for people with custom jQuery builds excluding event aliases much easier.

@cvrebert
Copy link
Collaborator

I would recommend removing the minified files from the commit, as they're likely to prevent clean auto-merging.

@cvrebert cvrebert added this to the v3.2.0 milestone Feb 16, 2014
@cvrebert cvrebert added the js label Feb 16, 2014
Makes life for people with custom jQuery builds excluding event aliases much easier.
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Feb 16, 2014

Done.

@zlatanvasovic
Copy link
Contributor

Is this really needed? .focus() is cleaner syntax than .trigger('focus').

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Feb 17, 2014

Well, it is not needed if you use a jQuery build which excludes nothing.
Given that in all of Bootstrap's JavaScript the .on(eventName, handler) syntax is used instead of the aliases it makes sense to also trigger those events via the direct syntax and not the alias. Also,
if you take a look at the source of $.fn.focus you can see that by calling the alias you're essentially wasting a function call.

@zlatanvasovic
Copy link
Contributor

Yes, but performances are very similar in both cases.

2014-02-17 19:20 GMT+01:00 Heinrich Fenkart [email protected]:

Well, it is not needed if you use a jQuery build which excludes nothing.
Given that in all of Bootstrap's JavaScript the .on(eventName, handler)syntax is used instead of the aliases it makes sense to also trigger those
events via the direct syntax and not the alias. Also,
If you take a look at the sourcehttp://james.padolsey.com/jquery/#v=git&fn=focusof
$.fn.focus you can see that by calling the alias you're essentially
wasting a function call.


Reply to this email directly or view it on GitHubhttps://github.com//pull/12761#issuecomment-35309527
.

Zlatan Vasović - ZDroid

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Feb 17, 2014

I know it is ridiculous to talk about performance on such a scale but still, the difference exists albeit it being relatively small. That is also why it was my second point and I tried to put more emphasis on the consistency argument.

@zlatanvasovic
Copy link
Contributor

OK so far.

2014-02-17 20:15 GMT+01:00 Heinrich Fenkart [email protected]:

I know it is ridiculous to talk about performance on such a scale but
still, the difference exists albeit it being relatively small. That is also
why it was my second point and I tried to put more emphasis on the
consistency argument.


Reply to this email directly or view it on GitHubhttps://github.com//pull/12761#issuecomment-35313866
.

Zlatan Vasović - ZDroid

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Feb 20, 2014

Any news on this? It's not that big of a change and would really help me out.

@mdo
Copy link
Member

mdo commented Feb 21, 2014

Sounds legit enough.

mdo added a commit that referenced this pull request Feb 21, 2014
Remove event aliases from JavaScript
@mdo mdo merged commit 70b875c into twbs:master Feb 21, 2014
@mdo mdo mentioned this pull request Feb 21, 2014
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Feb 22, 2014

❤️

@hnrch02 hnrch02 deleted the no-event-aliases branch February 22, 2014 09:57
@zlatanvasovic
Copy link
Contributor

This kinda breaks semver.

2014-02-22 10:59 GMT+01:00 Heinrich Fenkart [email protected]:

[image: ❤️]


Reply to this email directly or view it on GitHubhttps://github.com//pull/12761#issuecomment-35799133
.

Zlatan Vasović - ZDroid

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Feb 22, 2014

Care to elaborate in which way it should?

@zlatanvasovic
Copy link
Contributor

This removes .focus() to it breaks backwards-compatibility.

2014-02-22 14:18 GMT+01:00 Heinrich Fenkart [email protected]:

Care to elaborate in which way it should?


Reply to this email directly or view it on GitHubhttps://github.com//pull/12761#issuecomment-35802286
.

Zlatan Vasović - ZDroid

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Feb 22, 2014

No, it does not. When no parameter is passed to $.fn.focus, it is just a shortcut for $.fn.trigger('focus'), when the parameter is a function, it's a shortcut for $.fn.on('focus', parameter). See also jquery/src/event/alias.js

@zlatanvasovic
Copy link
Contributor

😕

@mdo
Copy link
Member

mdo commented Feb 22, 2014

So, not a problem then?

@zlatanvasovic
Copy link
Contributor

I don't know, I just know that this removes .focus() alias, so that changes
API and usage.

2014-02-22 22:37 GMT+01:00 Mark Otto [email protected]:

So, not a problem then?


Reply to this email directly or view it on GitHubhttps://github.com//pull/12761#issuecomment-35815567
.

Zlatan Vasović - ZDroid

@mdo
Copy link
Member

mdo commented Feb 22, 2014

Maybe @cvrebert can weigh in—I'd like more opinions on this.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Feb 22, 2014

There is no API change and no usage change, we just use a different method of the jQuery API that does exactly the same thing. I don't know how to convince you further, maybe someone else wanna give it a go?

/cc @cvrebert @fat

@zlatanvasovic
Copy link
Contributor

OK then. You can leave it if you'd want. :)

@cvrebert
Copy link
Collaborator

It's A-OK AFAIK.

cvrebert added a commit that referenced this pull request Mar 6, 2015
To ensure that we don't accidentally use any of the aliases.
This should prevent any future regressions from #12761.
Also updates the test suite since it now can't use these aliases either.
cvrebert added a commit that referenced this pull request Mar 6, 2015
To ensure that we don't accidentally use any of the aliases.
This should prevent any future regressions from #12761.
Also updates the test suite since it now can't use these aliases either.
cvrebert added a commit that referenced this pull request Mar 9, 2015
To ensure that we don't accidentally use any of the aliases.
This should prevent any future regressions from #12761.
Also updates the test suite since it now can't use these aliases either.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants