Change `fnBind` to a normal lib function instead of a polyfill #1278

Merged
merged 1 commit into from Apr 23, 2014

Projects

None yet

3 participants

@stucox
Member
stucox commented Mar 25, 2014

I know we document it, but it’s pretty hidden and might surprise some people. Also it could cause the fn.bind detect to fail (haven’t checked this though) – and generally I don’t think Modernizr should be changing the environment that low down, certainly not without an explicit opt-in.

It was added way back in #478, but that ticket ended up just discussing the API for prefixed() and didn’t discuss whether to polyfill or implement as a library function.

Also relevant: this chap reckons polyfills shouldn’t be AMD dependencies – and I think I agree. Maybe.

@ryanseddon
Member

Yep makes sense to me. Good one.

@ryanseddon ryanseddon commented on an outdated diff Mar 26, 2014
src/fnBind.js
- var F = function(){};
- F.prototype = target.prototype;
- var self = new F();
+ if (fn instanceof bound) {
+ console.log('bound');
@ryanseddon
ryanseddon Mar 26, 2014 Member

remove please

@ryanseddon ryanseddon and 1 other commented on an outdated diff Mar 26, 2014
src/fnBind.js
- return target.apply(
- that,
- args.concat(slice.call(arguments))
- );
+ } else {
+ console.log('not bound');
+ console.log(that);
+ console.log(slice.call(arguments));
+ /* jshint -W087 */
+ debugger;
@ryanseddon
ryanseddon Mar 26, 2014 Member

Also these

@stucox
stucox Mar 26, 2014 Member

Bah, I removed them but failed to commit/push! Done.

@stucox
Member
stucox commented Mar 26, 2014

Left over debugging removed.

@patrickkettner
Member

lgtm

@ryanseddon ryanseddon and 1 other commented on an outdated diff Mar 27, 2014
src/fnBind.js
- if (!Function.prototype.bind) {
- Function.prototype.bind = function bind(that) {
+ function fnBind (fn, that) {
@ryanseddon
ryanseddon Mar 27, 2014 Member

Should we have a check for bind and use it if available otherwise fallback to this.

@stucox
stucox Mar 27, 2014 Member

That sounds like the kind of thing a feature detection library would do, yes. I’ll update the PR.

@stucox stucox self-assigned this Mar 28, 2014
@ryanseddon
Member

ping @stucox

@stucox stucox Change `fnBind` to a normal lib function instead of a polyfill
Removing Function.bind unit test

Removing unneded global declaration

Removing debug statements

Using native fn.bind if available
4e128f6
@stucox
Member
stucox commented Apr 22, 2014

Done!

@stucox stucox added this to the Modernizr v3 milestone Apr 22, 2014
@ryanseddon ryanseddon merged commit 72616be into Modernizr:master Apr 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jtangelder jtangelder added a commit to jtangelder/Modernizr that referenced this pull request Apr 24, 2014
@jtangelder jtangelder separated the functions, so it's more like #1278. should be faster in…
… theory
f3fa71c
@stucox stucox referenced this pull request May 21, 2014
Closed

v3.0 release notes #805

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment