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

Idea: replace component.observe with component.on('change') #1197

Closed
Rich-Harris opened this issue Mar 1, 2018 · 12 comments
Closed

Idea: replace component.observe with component.on('change') #1197

Rich-Harris opened this issue Mar 1, 2018 · 12 comments

Comments

@Rich-Harris
Copy link
Member

I'm not sure why this popped into my head, and I'm not sure if it's a good idea or not. But something about it appeals to me, so I thought I'd note it here and see what people think.

Currently, the way to monitor a value over time is to use component.observe:

component.observe('foo', (newFoo, oldFoo) => {
  console.log(`foo changed from ${oldFoo} to ${newFoo}`);
});

This is quite a nice mechanism that's useful in lots of ways. But I wonder if we could replace it with an event/lifecycle hook — instead of all this code, we could do this sort of thing:

<script>
  export default {
    onchange({ changed, state, oldstate }) {
      if (changed.foo) console.log(`foo changed from ${oldstate.foo} to ${state.foo}`);
    }
  };
</script>

Equivalently:

<script>
  export default {
    oncreate() {
      this.on('change', ({ changed, state, oldstate }) => {
        if (changed.foo) console.log(`foo changed from ${oldstate.foo} to ${state.foo}`);
      });
    }
  };
</script>

This event would fire immediately before an update was applied. We'd need an equivalent event that would fire immediately after an update was applied (onchanged?)

The pros:

  • It's more expressive (we don't currently have any mechanism for knowing when any state changes have taken place, just when specific values have changed)
  • component.observe could be implemented in userland (i.e. svelte-extras) fairly easily. The implementation of observeMany would get much simpler.
  • It's more declarative — it would reduce the need for Add declarative way to set up observers and event listeners #1042 (though the point about custom event listeners is still valid), and would generally result in the user writing less code, I think
  • The compiler generates less code

The cons:

  • It's a breaking change
  • 'Observers' are a better way (than events) to conceptualise the idea of a specific value changing over time, and maybe that deserves to be represented in the API
  • The naming is hard. onchange and onchanged might be too confusingly similar. beforechange and afterchange or willchange and didchange don't have that problem, but I don't love them

None of this is high priority but I'd be curious if anyone has thoughts on this.

@ekhaled
Copy link
Contributor

ekhaled commented Mar 2, 2018

Prefer onchange and afterchange.

Can we keep both ways of doing it instead of making it a breaking change?
I can see both ways of doing it making sense depending on certain scenarios.

observe can always be deprecated over time.

In terms of generated code... Assuming the stuff in shared.js can be tree-shaken out, is there a lot of saving from the components themselves?

@Conduitry
Copy link
Member

My initial concern with this was going to be that it would remove Svelte's ability to do some compile-time optimizations, but then I realized that with observe there's already not really any optimizations it can do - any consumer of the component can attach whatever observer it wants.

However, having a single onchange hook does seem to be a bit hostile to using the components in a non-Svelte context (or as a top-level component that you want to interact with in some way). I guess in that case, it would be up to the component's onchange and onchange hook definitions to provide some sort of interface for that.

This seems to be one case where having them emitted as regular old events (rather than as a dedicated hook) would make this easier, as any interested party could subscribe to changes. Actually, now that I think of it, that would be helpful for communication between two Svelte components as well, because there are going to be some cases where using two-way binding isn't perfect.

I do like the onchange hook for its simplicity of syntax. Perhaps that could also be supported, and just be sugar for the latter this.on('change',...)? Not sure where that's a bad idea.

@Rich-Harris
Copy link
Member Author

Can we keep both ways of doing it instead of making it a breaking change?

Pre-v2, yes, I think we'd keep both, but observe would cause a deprecation warning and you'd be encouraged to use the version from svelte-extras.

In terms of generated code... Assuming the stuff in shared.js can be tree-shaken out, is there a lot of saving from the components themselves?

Because observe is a method of the prototype, it's always present even if you're not actually using it, so it doesn't get shaken out (because that would require some next-level static analysis — we still have ambitions to do that sort of thing in Rollup, but it's really hard). So in order to get the benefit, it does need to be removed from the core, hence the breaking v2 change.

This is the kind of change we're talking about, for the hello world component:

/* App.html generated by Svelte v1.56.0 */
var App = (function() { "use strict";

  function create_main_fragment(component, state) {
    var h1, text, text_1;

    return {
      c: function create() {
        h1 = createElement("h1");
        text = createText("Hello ");
        text_1 = createText(state.name);
      },

      m: function mount(target, anchor) {
        insertNode(h1, target, anchor);
        appendNode(text, h1);
        appendNode(text_1, h1);
      },

      p: function update(changed, state) {
        if (changed.name) {
          text_1.data = state.name;
        }
      },

      u: function unmount() {
        detachNode(h1);
      },

      d: noop
    };
  }

  function App(options) {
    init(this, options);
    this._state = assign({}, options.data);

    this._fragment = create_main_fragment(this, this._state);

    if (options.target) {
      this._fragment.c();
      this._fragment.m(options.target, options.anchor || null);
    }
  }

  assign(App.prototype, {
     destroy: destroy,
     get: get,
     fire: fire,
-     observe: observe,
     on: on,
     set: set,
     teardown: destroy,
     _set: _set,
     _mount: _mount,
     _unmount: _unmount,
     _differs: _differs
   });

  App.prototype._recompute = noop;

  function createElement(name) {
    return document.createElement(name);
  }

  function createText(data) {
    return document.createTextNode(data);
  }

  function insertNode(node, target, anchor) {
    target.insertBefore(node, anchor);
  }

  function appendNode(node, target) {
    target.appendChild(node);
  }

  function detachNode(node) {
    node.parentNode.removeChild(node);
  }

  function noop() {}

  function init(component, options) {
-    component._observers = { pre: blankObject(), post: blankObject() };
    component._handlers = blankObject();
    component._bind = options._bind;

    component.options = options;
    component.root = options.root || component;
    component.store = component.root.store || options.store;
  }

  function assign(target) {
    var k,
      source,
      i = 1,
      len = arguments.length;
    for (; i < len; i++) {
      source = arguments[i];
      for (k in source) target[k] = source[k];
    }

    return target;
  }

  function destroy(detach) {
    this.destroy = noop;
    this.fire('destroy');
    this.set = this.get = noop;

    if (detach !== false) this._fragment.u();
    this._fragment.d();
    this._fragment = this._state = null;
  }

  function get(key) {
    return key ? this._state[key] : this._state;
  }

  function fire(eventName, data) {
    var handlers =
      eventName in this._handlers && this._handlers[eventName].slice();
    if (!handlers) return;

    for (var i = 0; i < handlers.length; i += 1) {
      handlers[i].call(this, data);
    }
  }

-  function observe(key, callback, options) {
-    var group = options && options.defer
-      ? this._observers.post
-      : this._observers.pre;
-
-    (group[key] || (group[key] = [])).push(callback);
-
-    if (!options || options.init !== false) {
-      callback.__calling = true;
-      callback.call(this, this._state[key]);
-      callback.__calling = false;
-    }
-
-    return {
-      cancel: function() {
-        var index = group[key].indexOf(callback);
-        if (~index) group[key].splice(index, 1);
-      }
-    };
-  }

  function on(eventName, handler) {
    if (eventName === 'teardown') return this.on('destroy', handler);

    var handlers = this._handlers[eventName] || (this._handlers[eventName] = []);
    handlers.push(handler);

    return {
      cancel: function() {
        var index = handlers.indexOf(handler);
        if (~index) handlers.splice(index, 1);
      }
    };
  }

  function set(newState) {
    this._set(assign({}, newState));
    if (this.root._lock) return;
    this.root._lock = true;
    callAll(this.root._beforecreate);
    callAll(this.root._oncreate);
    callAll(this.root._aftercreate);
    this.root._lock = false;
  }

  function _set(newState) {
    var oldState = this._state,
      changed = {},
      dirty = false;

    for (var key in newState) {
      if (this._differs(newState[key], oldState[key])) changed[key] = dirty = true;
    }
    if (!dirty) return;

    this._state = assign({}, oldState, newState);
    this._recompute(changed, this._state);
    if (this._bind) this._bind(changed, this._state);

    if (this._fragment) {
-      dispatchObservers(this, this._observers.pre, changed, this._state, oldState);
+      this.fire("change", { changed: changed, state: this._state, oldState: oldState });
      this._fragment.p(changed, this._state);
-      dispatchObservers(this, this._observers.post, changed, this._state, oldState);
+      this.fire("changed", { changed: changed, state: this._state, oldState: oldState });
    }
  }

  function _mount(target, anchor) {
    this._fragment.m(target, anchor);
  }

  function _unmount() {
    if (this._fragment) this._fragment.u();
  }

  function _differs(a, b) {
    return a != a ? b == b : a !== b || ((a && typeof a === 'object') || typeof a === 'function');
  }

  function blankObject() {
    return Object.create(null);
  }

  function callAll(fns) {
    while (fns && fns.length) fns.shift()();
  }

-  function dispatchObservers(component, group, changed, newState, oldState) {
-    for (var key in group) {
-      if (!changed[key]) continue;
-
-      var newValue = newState[key];
-      var oldValue = oldState[key];
-
-      var callbacks = group[key];
-      if (!callbacks) continue;
-
-      for (var i = 0; i < callbacks.length; i += 1) {
-        var callback = callbacks[i];
-        if (callback.__calling) continue;
-
-        callback.__calling = true;
-        callback.call(component, newValue, oldValue);
-        callback.__calling = false;
-      }
-    }
-  }
  return App;
}());

That's a minified reduction from 2628 to 2174 bytes — zipped we go from 1068 bytes to 902 (under a kilobyte!) That's great news for standalone components.

This seems to be one case where having them emitted as regular old events (rather than as a dedicated hook) would make this easier, as any interested party could subscribe to changes.

Yep, that's exactly what I envisaged — it'd be handled the same way the ondestroy hook is currently handled:

this._handlers.destroy = [ondestroy];
this._handlers.change = [onchange];

So you'd be able to do component.on('change', ...) just as you can do component.on('destroy', ...).

@Rich-Harris
Copy link
Member Author

Though I suppose if this really was to be an observe replacement, the hook would also need to fire at the same time as oncreate (maybe change is called before create and changed is called immediately after?)

@ekhaled
Copy link
Contributor

ekhaled commented Mar 2, 2018

Pre-v2, yes, I think we'd keep both, but observe would cause a deprecation warning and you'd be encouraged to use the version from svelte-extras.

I'm sold on that 😄

@arxpoetica
Copy link
Member

Though I suppose if this really was to be an observe replacement, the hook would also need to fire at the same time as oncreate (maybe change is called before create and changed is called immediately after?)

Or just pass a var to onchange that tells you it's init—that feels gross to me actually. But so does the duplication of change and changed...

@TehShrike
Copy link
Member

I like it. But please, beforechange and afterchange - no onchange - or I'll always be looking it up!

@Rich-Harris
Copy link
Member Author

I was actually wondering if it should be onstate and onupdate or something... needs to be considered in the context of #1234 (which I haven't yet provided any feedback on — sorry @aphitiel!). The nice thing about the on prefix is that ondestroy is basically just shorthand for this.on('destroy', ...) and this would be similar.

But it's very much open to bikeshedding.

@arxpoetica
Copy link
Member

arxpoetica commented Mar 31, 2018

My vote is for on[whatever] for the naming, both for consistency, as well as for the potential alternatives this.on([whatever]...).

@TehShrike
Copy link
Member

onbeforechange and onafterchange then :-x

@Rich-Harris Rich-Harris mentioned this issue Apr 14, 2018
@Rich-Harris
Copy link
Member Author

About to start work on this. My current thinking:

<script>
  export default {
    onstate({ changed, current, previous }) {
      // this is called before oncreate (previous === undefined)
      // and on every state change
    },

    oncreate() {
      // this is called once, when the component is created, after onstate
    },

    onupdate({ changed, current, previous }) {
      // this is called after oncreate (previous === undefined)
      // and AFTER every state change, when the DOM is up to date
    },

    ondestroy() {
      // this is called once, when the component is destroyed
    }
  };
</script>

A userland implementation of observe would look something like this:

<script>
  export default {
    methods: {
      observe(prop, callback, opts = {}) {
        if (opts.init !== false) {
          callback(this.get()[prop], undefined);
        }

        return this.on(opts.defer ? 'update' : 'state', ({ changed, current, previous }) => {
          if (changed[prop]) callback(current[prop], previous[prop]);
        });
      }
    }
  };
</script>

I'm suggesting onstate instead of onbeforechange partly because it's much less to type, but partly because 'change' doesn't make sense if it fires before oncreate (which it should, I think — this is a useful thing that we currently lack).

Rich-Harris added a commit that referenced this issue Apr 15, 2018
Rich-Harris added a commit that referenced this issue Apr 15, 2018
Implement onstate and onupdate hooks
@Rich-Harris
Copy link
Member Author

This is implemented in 1.63. Just need to update the docs and svelte-extras

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

5 participants