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

Batched bindings #760

Closed
Rich-Harris opened this issue Aug 10, 2017 · 3 comments
Closed

Batched bindings #760

Rich-Harris opened this issue Aug 10, 2017 · 3 comments

Comments

@Rich-Harris
Copy link
Member

Rich-Harris commented Aug 10, 2017

Bindings on components are implemented with component.observe. That's convenient enough, but it has an awkward consequence — if you have two bindings, and they both change in the same set operation, you end up with two distinct updates. That's obviously somewhat wasteful.

Consider this example:

<!-- App.html -->
<Foo bind:bar bind:baz/>
<p>bar: {{bar}}</p>
<p>baz: {{baz}}</p>

<script>
  import Foo from './Foo.html';
  
  export default {
    components: {
      Foo
    }
  };
</script>

<!-- Foo.html -->
<p>bar in Foo: {{bar}}</p>
<p>baz in Foo: {{baz}}</p>
<button on:click='double()'>double</button>

<script>
  export default {
    data() {
      return {
        bar: 1,
        baz: 2
      }
    },
    
    methods: {
      double() {
        const { bar, baz } = this.get();
        this.set({ bar: bar * 2, baz: baz * 2 });
      }
    }
  };
</script>

Every time you hit the 'double' button, the app has to update twice.

We could fix this by implementing bindings differently. This is some untested code that probably misses certain important scenarios, but I think something like this is probably on the right lines:

import Foo from './Foo.html';

function create_main_fragment ( state, component ) {
-  var foo_updating = false, foo_updating_1 = false, text, p, text_1, text_2_value, text_2, text_3, p_1, text_4, text_5_value, text_5;
+  var foo_updating = false, text, p, text_1, text_2_value, text_2, text_3, p_1, text_4, text_5_value, text_5;

  var foo_initial_data = {};
  if ( 'bar' in state ) foo_initial_data.bar = state.bar ;
  if ( 'baz' in state ) foo_initial_data.baz = state.baz;
  var foo = new Foo({
    _root: component._root,
+    _bind: function( changed ) {
+      var data = {}, state = component.get(), dirty = false;
+      if ( 'bar' in changed ) { data.bar = changed.bar; dirty = true; }
+      if ( 'baz' in changed ) { data.baz = changed.baz; dirty = true; }
+      if ( dirty ) {
+        foo_updating = true;
+        component._set( data );
+        foo_updating = false;
+      }
+    },
    data: foo_initial_data
  });

-  function observer ( value ) {
-    if ( foo_updating ) return;
-    foo_updating = true;
-    component.set({ bar: value });
-    foo_updating = false;
-  }

-  foo.observe( 'bar', observer, { init: false });

-  component._root._beforecreate.push( function () {
-    var value = foo.get( 'bar' );
-    if ( differs( value, state.bar  ) ) {
-      observer.call( foo, value );
-    }
-  });

-  function observer_1 ( value ) {
-    if ( foo_updating_1 ) return;
-    foo_updating_1 = true;
-    component.set({ baz: value });
-    foo_updating_1 = false;
-  }

-  foo.observe( 'baz', observer_1, { init: false });

-  component._root._beforecreate.push( function () {
-    var value = foo.get( 'baz' );
-    if ( differs( value, state.baz ) ) {
-      observer_1.call( foo, value );
-    }
-  });

  foo._context = {
    state: state
  };

  return {
    create: function () {
      foo._fragment.create();
      text = createText( "\n" );
      p = createElement( 'p' );
      text_1 = createText( "bar: " );
      text_2 = createText( text_2_value = state.bar );
      text_3 = createText( "\n" );
      p_1 = createElement( 'p' );
      text_4 = createText( "baz: " );
      text_5 = createText( text_5_value = state.baz );
    },

    mount: function ( target, anchor ) {
      foo._fragment.mount( target, anchor );
      insertNode( text, target, anchor );
      insertNode( p, target, anchor );
      appendNode( text_1, p );
      appendNode( text_2, p );
      insertNode( text_3, target, anchor );
      insertNode( p_1, target, anchor );
      appendNode( text_4, p_1 );
      appendNode( text_5, p_1 );
    },

    update: function ( changed, state ) {
+      if ( !foo_updating ) {
+        var data = {}, dirty = false;
+        if ( 'bar' in changed ) { data.bar = changed.bar; dirty = true; }
+        if ( 'baz' in changed ) { data.baz = changed.baz; dirty = true; }
+        if ( dirty ) {
+          foo_updating = true;
+          component._set( data );
+          foo_updating = false;
+        }
+      }
      
-      if ( !foo_updating && 'bar' in changed ) {
-        foo_updating = true;
-        foo._set({ bar: state.bar  });
-        foo_updating = false;
-      }

-      if ( !foo_updating_1 && 'baz' in changed ) {
-        foo_updating_1 = true;
-        foo._set({ baz: state.baz });
-        foo_updating_1 = false;
-      }

      foo._context.state = state;

      if ( text_2_value !== ( text_2_value = state.bar ) ) {
        text_2.data = text_2_value;
      }

      if ( text_5_value !== ( text_5_value = state.baz ) ) {
        text_5.data = text_5_value;
      }
    },

    unmount: function () {
      foo._fragment.unmount();
      detachNode( text );
      detachNode( p );
      detachNode( text_3 );
      detachNode( p_1 );
    },

    destroy: function () {
      foo.destroy( false );
    }
  };
}

Then, we call this._bind inside _set. Which itself could arguably be improved by checking that values were in fact changed:

App.prototype._set = function _set ( newState ) {
  var oldState = this._state;
+  var changed = {}, dirty = false;
+  for ( var key in newState ) {
+    if ( differs( newState[key], oldState[key] ) ) { changed[key] = newState[key]; dirty = true; }
+  }
+  if ( !dirty ) return;
-  this._state = assign( {}, oldState, newState );
-  dispatchObservers( this, this._observers.pre, newState, oldState );
-  this._fragment.update( newState, this._state );
-  dispatchObservers( this, this._observers.post, newState, oldState );
+  this._state = assign( {}, oldState, changed );
+  if ( this._bind ) this._bind( changed );
+  dispatchObservers( this, this._observers.pre, changed, oldState );
+  this._fragment.update( changed, this._state );
+  dispatchObservers( this, this._observers.post, changed, oldState );
};

(Though perhaps the dirty check belongs in set and not _set? Not actually sure.)

@Rich-Harris
Copy link
Member Author

One thing to note with this approach — any changes inside <Foo> would trigger this._bind(...)might be better to tell the child which keys the parent is interested in.

@kzc
Copy link

kzc commented Aug 10, 2017

You've probably caught this already - dirty will always be true:

+        if ( 'bar' in changed ) data.bar = changed.bar; dirty = true;
+        if ( 'baz' in changed ) data.baz = changed.baz; dirty = true;
+        if ( dirty ) {

@Rich-Harris
Copy link
Member Author

No, I hadn't 😀 That's what I get for trying to write code and eat breakfast at the same time. Will fix the code so I don't confuse myself later (though it probably has other bugs, such as not initialising bindings where the parent doesn't have a value but the child does). Thanks

This was referenced Aug 14, 2017
Rich-Harris added a commit that referenced this issue Aug 16, 2017
Implement batched bindings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants