Fix #1762: Implement faster bind function #1782

darkwing · 2016-08-12T03:14:43Z

One idea I had and have seen before is, instead of this.blah, this all the time, doing 'blah', this and assuming that this holds the function if the first argument is a string. i.e:

module.exports.bind = function (fn, ctx) {
  if(typeof fn === 'string') fn = ctx[fn];
  return function bound () {
    return fn.apply(ctx, arguments);
  };
};

bind('onKeyUp', this);

ngokevin · 2016-08-12T14:40:46Z

Can we get some unit test coverage on it?

darkwing · 2016-08-12T14:44:22Z

Yep; left off late at night. Will add tests today!

ngokevin · 2016-08-12T14:47:27Z

src/utils/index.js

 module.exports.debug = require('./debug');
 module.exports.entity = require('./entity');
 module.exports.forceCanvasResizeSafariMobile = require('./forceCanvasResizeSafariMobile');
+module.exports.functions = require('./functions');


Actually might be more consistent to have this as function rather than functions. (I imagine we'd have object and we already have entity). Hopefully not too much hassle to replace.

I don't think functions is a very good name. All modules export functions. I would just name the file bind.js an do require('./bind');

They'd be utility related to wrapping functions. I could imagine we'd add more function-related utilities in the future (e.g., debounce), and it'd be more organized and consistent to namespace them.

Agreed @ngokevin. We could add forEach (in some places we're doing [].forEach and others we're doing Array.prototype.forEach(NodeList), etc. I'm sure it will grow.

codecov-io · 2016-08-12T20:11:28Z

Current coverage is 82.05% (diff: 93.33%)

Merging #1782 into master will increase coverage by 0.12%

@@             master      #1782   diff @@
==========================================
  Files           100        101     +1   
  Lines          3548       3573    +25   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2907       2932    +25   
  Misses          641        641          
  Partials          0          0

Powered by Codecov. Last update fb09d80...d091397

dmarcos · 2016-08-12T20:13:18Z

src/components/camera.js

 var registerComponent = require('../core/component').registerComponent;
 var THREE = require('../lib/three');
 var utils = require('../utils/');
+var bind = utils.function.bind;


I still think that the function namespace does not convey anything. Just call it utils.bind

It conveys function binding. What would utils.bind bind?

I'd like it for organization and consistency above anything else though.

I don't think by itself it useful yeah. But I'd just like to be flexible in case we add some of http://underscorejs.org/#functions in the future.

If we add other functions in the future we can add the namespace. Let's keep it simple for now and not anticipate.

Could, but I don't understand why we wouldn't want to avoid a potential breaking change for the sake of a typing a few extra characters.

function resonates with you because you're familiarized with underscore. Without that context is meaningless.

We may or may not, but I think it's smart to prepare for the possible scenarios. If we don't ever add more functions, then it might look cluttered (subjectively, I think it's organized). If we do and we end up namespacing it later, we have to call a breaking change and components using the function (possibly third-party) have to be migrated.

I wouldn't assume what resonates with me. It makes sense to me because the function utilities wrap functions, and there are various use cases of wrapping functions.

If you code trying to anticipate everything you will have super abstract unmaintainable code with optimizations that are there for no reason. Let's just add a bind file and generalize, create a namespace when there's a need.

As I side note. We should not make those functions part of the public API to avoid the liability and have smaller API surface

darkwing · 2016-08-12T20:19:32Z

Test added.

ngokevin · 2016-08-12T20:23:32Z

tests/utils/function.test.js

+        return this.propName;
+      }
+    };
+    assert.equal(obj.getProp(), bind(obj.getProp, obj)());


Would it be useful to have a test that binds to a different obj with different value?

darkwing · 2016-08-17T23:01:52Z

src/utils/bind.js

+/**
+ * Faster version of Function.prototype.bind
+ */
+module.exports = function bind (fn, ctx/* , arg1, arg2 */) {


This function got more complicated because it doesn't accommodate for passing more arguments to bind, i.e. bind(fn, context, VALUE1, VALUE2). So I had to update it. :/

We use the extra VALUE1 in light.js

Ah, good to have.

We can document this in the docstring

* @param {Function} fn - Function to wrap. * @param {Object} ctx - What to bind as context. * @param {...*} arguments - Arguments to pass through.

darkwing · 2016-08-18T00:55:30Z

Updates made!

mkungla added the needs review label Aug 12, 2016

ngokevin reviewed Aug 12, 2016
View reviewed changes

dmarcos added needs changes and removed needs review labels Aug 12, 2016

darkwing force-pushed the 1762-bind branch from b626c5c to 0b3b9d3 Compare August 12, 2016 20:06

dmarcos reviewed Aug 12, 2016
View reviewed changes

ngokevin reviewed Aug 12, 2016
View reviewed changes

darkwing force-pushed the 1762-bind branch 2 times, most recently from 67513f5 to d5e8831 Compare August 12, 2016 21:57

cvan added the needs rebase label Aug 16, 2016

darkwing force-pushed the 1762-bind branch from d5e8831 to a6719a2 Compare August 17, 2016 23:00

darkwing reviewed Aug 17, 2016
View reviewed changes

ngokevin added needs review and removed needs changes needs rebase labels Aug 17, 2016

Fix aframevr#1762: Implement faster bind function

d091397

darkwing force-pushed the 1762-bind branch from a6719a2 to d091397 Compare August 18, 2016 00:54

ngokevin merged commit f9c64ee into aframevr:master Aug 20, 2016

cvan removed the needs review label Aug 30, 2016

Uh oh!

Fix #1762: Implement faster bind function #1782

Fix #1762: Implement faster bind function #1782

Uh oh!

Conversation

darkwing commented Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngokevin commented Aug 12, 2016

Uh oh!

darkwing commented Aug 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmarcos Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 82.05% (diff: 93.33%)

Uh oh!

dmarcos Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngokevin Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngokevin Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmarcos Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkwing commented Aug 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngokevin Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkwing commented Aug 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

darkwing commented Aug 12, 2016 •

edited

Loading

dmarcos Aug 12, 2016 •

edited

Loading

codecov-io commented Aug 12, 2016 •

edited

Loading

dmarcos Aug 12, 2016 •

edited

Loading

ngokevin Aug 12, 2016 •

edited

Loading

ngokevin Aug 12, 2016 •

edited

Loading

dmarcos Aug 12, 2016 •

edited

Loading

ngokevin Aug 17, 2016 •

edited

Loading