consider more functional component handlers

[ngokevin]

Description:

Just a random idea.

• Rely less on this.
• Make functions more contained such that they can easily be adapted, unit tested, and more predictable. You wouldn't need to the DOM to test.
• Makes it more explicitly apparent what is available rather than a this namespace.
• If another framework wanted to adapt the A-Frame components, they could write an adapter for entity.
• By passing in state, modifying it, and returning it, we could have debug tools to keep a history of all state changes. (i.e., state machine). Perhaps open doors to composing/wrapping functions

AFRAME.registerComponent(‘foo’, {
  initialState: {
    currentPosition: new THREE.Vector3()
  },

  schema: {},

  update: function (entity, data, state, oldData) {
    state.currentPosition.x += 1;
    return state;
  }
});

[mflux]

I wanted to throw in a suggestion for this alternative:

  AFRAME.registerComponent('component', function( context ){
    //  context = this in the old use-case
    //  anything in this closure is "private" unless returned
    const path = context.data.path;        

    context.el.addEventListener( 'model-loaded', function( result ){
      //  no "this" confusion
    });

    function tick( dt ){
      //  define a tick, with access to everything private
    }

    function reset(){
      //  even custom functionality
    }

    //  return anything required for components
    return {
      schema: {
        type: 'string'
      }          
      tick,                    
      reset,
    }
  });

• The function passed in IS the init function. It returns the object normally required by components.
• Allows for private state to be shared between utility functions (update, init, tick).
• Avoids "this" entirely
• Can be implemented as a parallel path if need be: AFRAME.registerComponentFn( ... )
• Or AFRAME.registerComponent can check if the second arg is a function, run that function and use it as its init object, extending existing code.

One last tiny bonus: init function can be passed important things like this, el, renderer, etc so component writers don't need to guess how to deference things like renderer (this.el.sceneEl.renderer...). The init function can pass that ( function( context, scene, renderer ) ).

[dmarcos]

@mflux Can you articulate what this confusion mean? Simply passing the prototype of the component to registerComponent as we do know is simple and effective. Using this is pretty vanilla JS and every developer regardless of the library they come from understands it. In your example, It's not clear what context is without having to read extra docs. The current architecture is not opinionated and very flexible. You could wrap your variables in a closure if you prefer or you could even create your own registerComponent that implements the pattern you describe.

[dmarcos]

You can actually do what you describe... You can create AFRAME.registerComponentFn.

[ngokevin]

I think he means you wouldn't have to do method binding, doing var self = this, or doing like var x = this.x.

I can see the benefits. This is still vanilla JS, and you don't know what's in this either without reading the docs. At least with context, you can console.log it and read it cleanly. They're both equally flexible, this is just a bit cleaner. You'd also be able to create private "methods" that have still access to the scope, which currently you can't.

[ngokevin]

It's also prescribe developers to pre-define all their state/internal variables up top and discourage them from calling handler functions as if they were methods.

[dmarcos]

I like simplicity over all. @mflux design is for me a proof of success of keeping things simple and unopinionated. He comes to a-frame with his own particular background and was quickly able to successfully bend the current design to his own taste.

[mflux]

Yup :) This is what I proposed, registerComponentFn or have registerComponent check if the second argument is a function, or I just create a thirdparty thing and use it myself.

Anyway, I'm happy to articulate the issue and why I think this is an improvement. I first learned of writing JS without using this from Douglas Crockford in this talk (timestamped link).

I'll be the first to admit it's "opinionated". But hey, what interesting person have you ever met that doesn't have an opinion?

Issue 1: Callbacks and "this"

Here's a typical a-frame component:

  AFRAME.registerComponent('component', {        
    schema: {
      type: 'string'
    },
    init: function(){
    }
  });

Any time I want to get "data", I have to access "this". Say, then, I need to write callbacks and then use "data":

      this.el.addEventListener( 'model-loaded', function( event ){
        event.detail.model.material.color.setHex( this.data );
      });

Oops. "this" refers to the function callback. This is one of the most common pitfalls for beginners in JS. So instead, I may try to do the ES6 arrow function (or fall back to "bind" in older browsers):

      this.el.addEventListener( 'model-loaded', ( event )=>{
        event.detail.model.material.color.setHex( this.data );
      });

A common strategy is to defer "this" into "that"?

    const that = this;
    this.el.addEventListener( 'model-loaded', function( event ){
      event.detail.model.material.color.setHex( that.data );
    });

That's a bit confusing to read since you have to have two separate "this" to keep track of in your head. Instead, let's simply deference what we need:

      const data = this.data;
      this.el.addEventListener( 'model-loaded', function( event ){
        event.detail.model.material.color.setHex( data );
      });

But why do this deferencing? Couldn't a function just have passed me this data?

    function( data ){
      this.el.addEventListener( 'model-loaded', function( event ){
        event.detail.model.material.color.setHex( data );
      });
    }

In fact, let's go a step further:

    function( el, data ){
      el.addEventListener( 'model-loaded', function( event ){
        event.detail.model.material.color.setHex( data );
      });
    }

Boom. This is totally gone. Was there ever a need for "this" to begin with? Other than attaching everything in it as a bundle? Instead, if we're fancy with ES6 destructuring:

    function( { el, data } ){          
      el.addEventListener( 'model-loaded', function( event ){
        event.detail.model.material.color.setHex( data );
      });
    }        

That's a single object (what used to be "this") passed into the argument parameter (context) and now we have both el and data, free from deferencing.

Issue 2: Sharing State

Back to our initial example, we're adding a tick function to our component:

  AFRAME.registerComponent('component', {
    schema: {
      type: 'string'
    },
    init: function(){
    }
    tick: function(){
    }
  });

Our component needs an animation mixer:

    init: function(){
      const mixer = new THREE.AnimationMixer();
      //  set it up
    }      

Tick needs to access it, but has no way of doing so. We've been forced to attach it to the only shared space, "this":

    init: function(){
      this.mixer = new THREE.AnimationMixer();
      //  set it up
    }

    tick: function( dt ){
      this.mixer.update( dt );
    }

In a closure, this is very easy to understand:

    function init(){
      const mixer = new THREE.AnimationMixer();
      function tick( dt ){
        mixer.update( dt );
      }
      return {
        tick
      };
    }

This has the added bonus of hiding the mixer (private, in the closure) until users of this component absolutely has to have it. If it turns out we need to expose it, we can do this:

    function init(){
      const mixer = new THREE.AnimationMixer();
      function tick( dt ){
        mixer.update( dt );
      }
      return {
        tick,
        mixer
      };
    }        

Anyway, I don't really have a dog in this race. This is just my 2cents, and how I write code, and yes it's my opinion that it's far superior to using this unless it's for performance reasons.

[mflux]

So I went ahead and tried it myself to see if it was feasible. It, kind of is?

Here's my fork: https://github.com/mflux/aframe Not sure how you're supposed to try forks. I'll leave that up to you.

There's an example that uses this: https://github.com/mflux/aframe/tree/master/examples/boilerplate/hello-world-componentfn

My modified registerComponentFn function:

module.exports.registerComponentFn = function (name, fn) {
  var NewComponent;
  var proto = {};

  const definition = fn();

  if (name.indexOf('__') !== -1) {
    throw new Error('The component name `' + name + '` is not allowed. ' +
                    'The sequence __ (double underscore) is reserved to specify an id' +
                    ' for multiple components of the same type');
  }

  // Format definition object to prototype object.
  Object.keys(definition).forEach(function (key) {
    proto[key] = {
      value: definition[key],
      writable: true
    };
  });

  if (components[name]) {
    throw new Error('The component `' + name + '` has been already registered. ' +
                    'Check that you are not loading two versions of the same component ' +
                    'or two different components of the same name.');
  }
  NewComponent = function (el, attr, id) {
    Component.call(this, el, attr, id);
    const methods = this.setup({
      context: this,
      el,
      attr,
      id,
      sceneEl: el.sceneEl,
      scene: el.sceneEl.object3D,
      object3D: el.object3D,
      data: buildData(el, this.name, this.schema, attr)
    });

    for (let key in methods) {
      this[ key ] = methods[ key ];
    }

    if (!el.hasLoaded) { return; }
    this.updateProperties(this.attrValue);
  };

  NewComponent.prototype = Object.create(Component.prototype, proto);
  NewComponent.prototype.name = name;
  NewComponent.prototype.constructor = NewComponent;
  NewComponent.prototype.system = systems && systems.systems[name];
  NewComponent.prototype.play = wrapPlay(NewComponent.prototype.play);
  NewComponent.prototype.pause = wrapPause(NewComponent.prototype.pause);

  components[name] = {
    Component: NewComponent,
    dependencies: NewComponent.prototype.dependencies,
    multiple: NewComponent.prototype.multiple,
    parse: NewComponent.prototype.parse,
    parseAttrValueForCache: NewComponent.prototype.parseAttrValueForCache,
    schema: utils.extend(processSchema(NewComponent.prototype.schema)),
    stringify: NewComponent.prototype.stringify,
    type: NewComponent.prototype.type
  };
  return NewComponent;
};

Some notes:

• This only works because I had access to Component.prototype. Since it's not exposed anywhere normally on AFrame's namespace, I couldn't write a stand-alone version. If AFrame exposed Component.prototype, I could write a version that's used by third-parties without any other modification to AFrame.
• The function that's passed into AFRAME.registerComponentFn can't actually be the init function because at this stage we're still registering the component. To get around this, I created a "setup" function that gets returned (along with schema). It's not perfect but it works.
• Setup is called on the NewComponent's constructor. Setup returns an init, tick, and anything else and over-writes the prototypes defaults. This is not as fast as calling prototypes, as benchmarks will tell you, but it's good enough for me.
• I couldn't figure out how to initialize buildData for multi-variable schemas. It just doesn't work until I hit updateProperties (but that calls init() so I can't do that before my setup()).

Here's example component:

AFRAME.registerComponentFn('test-component', function(){

  function setup( {context, el, attr, id, scene, object3D, data } ){

    console.log( el );
    console.log( object3D );
    console.log( scene );
    console.log( data );

    const mesh = el.getObject3D( 'mesh' );

    const clock = new THREE.Clock();
    const speed = data;

    function init(){

    }

    function tick( dt ){
      mesh.material.color.r = Math.abs( Math.sin( clock.getElapsedTime() * speed ) );
    }

    return {
      init,
      tick
    };
  }

  return {
    setup,
    schema: {
      type: 'float'
    }
  }
});

Notes:

• I really like writing components this way what can I say... AFrame can be configured to return a bunch of commonly used things to setup() and I just extract them in setup's function argument.
• Anything declared inside setup() is global only to the component itself and accessible by tick, play, pause, update, etc.
• Anything declared in the function passed into registerComponentFn is also global to all components of this type, which is really cool because you can have "systems" that handle load order, or batch meshes together of the same component type. You can have one OBJLoader for all obj-models here. You can also define constants for all same-type components.

My conclusions from this exercise:

• I haven't ran benchmarks but I'm willing to bet this will have a performance cost. This might be a deal-breaker for some people.
• IMO it's much more flexible and cleaner than "this" mess. It's functional, and uses language features better.
• A stand-alone version of this is impossible to write without small changes to AFrame (Component.prototype needs to be exposed... probably).

[ngokevin]

Awesome. At the very minimum, we can expose the Component prototype now.

I'd like to write components this way too as it would be much easier to unit test and decide what is private and public. Ideally, we could be able to pass a prototype or function into the same function while having both of them go down the same code path and not introduce extra API. Will have to explore that more.

Also reminds me a bit of React. All components were classes/prototypes, but then they introduced stateless functional components where you could define a component via just a function, input props and output the render props => <ReactComponent/>.

[donmccurdy]

Having used them in AngularJS, Express, and Koa, my opinion is that state management through state/context/scope arguments often introduces more inconsistency than it fixes, and I would prefer not to see that become a required pattern. The nuances of this are inherent to JavaScript, and the community is already dealing with those at the language level (fat arrows, class syntax, etc).

Exposing component prototype makes sense to me; I'm certainly supportive of letting devs write however they prefer.

[ngokevin]

The prototype is exposed now. We can create a third-party wrapper and try it out, test its ergonomics.

[ngokevin]

While I like the closure syntax more than our current state, we're on the road we're on...hopefully class syntax might help things later on.