.reference() for neverending cursors #30

torgeir · 2015-02-05T21:31:52Z

A PR is easier for cooperation

torgeir · 2015-02-05T21:33:06Z

This will allow for something like

var clock = quiz.reference('clock');

let Clock = component('Clock', [ forceUpdateOn(clock) ], () =>
  div({},
    String(clock.cursor().deref())));

function forceUpdateOn (ref) {
  var bound;
  return {
    componentDidMount: function () {
      bound = ref.onChange(_ => this.forceUpdate());
    },
    componentWillUnmount: function () {
      bound.remove();
    }
  };
}

as per omniscientjs/omniscient#42

mikaelbr · 2015-02-05T21:35:41Z

The current implementation is more like:

var clock = quiz.reference('clock');

let Clock = component('Clock', [ forceUpdateOn(clock) ], () =>
  div({},
    String(clock.cursor().deref())));

function forceUpdateOn (ref) {
  return {
    componentDidMount: function () {
      ref.onChange(_ => this.forceUpdate());
    },
    componentWillUnmount: function () {
      ref.unsubscribe();
    }
  };
}

(As it is unsubscribe on a reference level, not onChange-level)

mikaelbr · 2015-02-05T21:43:25Z

My main issue with this at this point is that it seems unnecessary to have many parallel equal listeners on "swap", instead of having one listener. One listener would make cleanup a more manual job, though.

I haven't tested the code extensively, nor have I written tests yet. This is just a concept / PoC, which we can discuss and possibly hack on. But initial run-through seems positive!

dashed · 2015-02-05T21:53:59Z

src/structure.js


  if (!this.current) {
    throw new Error('No structure loaded.');
  }

  var changeListener = function (newRoot, oldRoot, path) {
-
+    onChange(newRoot, oldRoot, path);


This should go onto its own changeListener function. i.e. changeListener = handleOnChange(this, changeListener);

Also, onChange(newRoot, oldRoot, path); shouldn't be called before self.current is set.

Yeah, agreed. This is just from early stages of experimentation and should probably not be in this PR, or maybe even be a part of the code at all. Should strive to only have relevant code and features we need.

dashed · 2015-02-05T22:02:17Z

Agree with @mikaelbr that registering the structure once with a centralized function is sufficient. I do something similar with my own immstruct wrapper: https://gist.github.com/Dashed/e38d4184bc239268737e#file-listen-js-L84-L120

dashed · 2015-02-05T22:13:34Z

src/structure.js

+  this.on('swap', changeListener);
+
+  return {
+    onChange: function (newFn) {


Will need a way to "unsubscribe" functions. Otherwise, repeated calls to componentDidMount will attach functions, which will be repeated executed, causing an unnecessary amounts of cascading of function calls.

I'm basing this on this example:

var clock = quiz.reference('clock'); let Clock = component('Clock', [ forceUpdateOn(clock) ], () => div({}, String(clock.cursor().deref()))); function forceUpdateOn (ref) { return { componentDidMount: function () { ref.onChange(_ => this.forceUpdate()); }, componentWillUnmount: function () { ref.unsubscribe(); } }; }

To clarify, fn reference still exists when calling onChange after calling unsubscribe.

Yes, this should probably be handled a bit different. Most likely, there's no need to pass in a function on reference creation when you have onChange as well.

torgeir · 2015-02-06T06:53:58Z

Afaik, unsubscribing one listener and stopping a ref from any longer emitting changes are two separate things. One component should be able to unsubscribe its ref listeners without stopping other component's listeners

mikaelbr · 2015-02-06T07:20:31Z

@torgeir For now it is on reference level, not affecting anyone other then themself, but having one listener in common for every reference, would require some more logic for unsubscribing on reference level, yeah.

torgeir · 2015-02-06T07:23:12Z

That's my point excactly, you should be able to cancel subscritions as well as tear a whole ref

Edit: could probably be solved by using a list of functions that are looped and called if the key is the same on swap. unsubscribe() could remove the function instance from the list, while cancel could empty the list and remove the swap listener.

dashed · 2015-02-06T08:01:35Z

Why can't we make the function subscription a little bit smarter? We track by reference counting.

We subscribe changeListener to swap only when function passed to onChange is the first subscription. We unsubscribe changeListener from swap only when function removed from viaunsubscribe is the last listener.

changeListener subscription should be automatic. We, the users, shouldn't be handling this manually.

All we want and care about is adding and removing functions via onChange and unsubscribe.

dashed · 2015-02-06T08:03:09Z

src/structure.js

+        return original.apply(this, arguments);
+      }
+    },
+    cursor: function () {


If we're going to have this, a dual structure() should be provided to get the structure reference.

dashed · 2015-02-07T03:40:41Z

I think Structure.prototype.reference should be renamed to something else. It may be misleading in that it refers to a static internal object associated with keyPath.

Imagine having var clock = quiz.reference('clock'); in two different components. Calling clock.unsubscribeAll() in one component doesn't affect clock object in the other component.

Maybe Structure.prototype.scope?

dashed · 2015-02-07T03:41:00Z

@mikaelbr looking good so far 👍

dashed · 2015-02-07T04:08:51Z

Since Immutable is available. I'm wondering if the collections can be used instead of native object/array.

mikaelbr · 2015-02-07T11:03:04Z

Since Immutable is available. I'm wondering if the collections can be used instead of native object/array.

I started out with immutable structure, but it didn't really make the code easier or improve it much. Don't see any real gain at this point.

mikaelbr · 2015-02-07T11:09:41Z

Maybe Structure.prototype.scope?

We had a discussion on this, but we found that .reference still is more descriptive. You can have multiple references to a everlasting cursor, and I actually think this makes sense:

Imagine having var clock = quiz.reference('clock'); in two different components. Calling clock.unsubscribeAll() in one component doesn't affect clock object in the other component.

So, you are unsubscribing on all listeners for that reference, it shouldn't remove listeners on other references. They aren't the same references, just all references to the same data.

dashed · 2015-02-07T11:27:16Z

So, you are unsubscribing on all listeners for that reference, it shouldn't remove listeners on other references. They aren't the same references, just all references to the same data.

I need to think on this some more; maybe it'll grow on me 😛

/cc @skevy Thoughts?

dashed · 2015-02-07T11:32:20Z

src/structure.js

+      var fnIndex, listenerNs = self._pathListeners[pathId];
+
+      self._pathListeners[pathId] = !listenerNs ? [newFn] : listenerNs.concat(newFn);
+      fnIndex = self._pathListeners[pathId].indexOf(newFn);


May be better to instead use self._pathListeners[pathId].length - 1.

dashed · 2015-02-07T11:46:50Z

onChange is pretty low-level. I feel that there should be sugar functions onAdd, onDelete, andonUpdate that wrap upon onChange.

// abstracted out
let analyze = function(newData, oldData, path) {

    var oldObject = oldData && oldData.getIn(path);
    var newObject = newData && newData.getIn(path);

    var inOld = oldData && hasIn(oldData, path);
    var inNew = newData && hasIn(newData, path);

    return {
        oldObject,
        newObject,
        inOld,
        inNew
    };
}

let onAdd = function(newFn) {

    return this.onChange(function(newData, oldData, path) {

        let ret = analyze(newData, oldData, path)
        if(!ret.inOld && ret.inNew) {
            newFn(path, ret.newObject)
        }
    })
}

// similar to onDelete and onUpdate

dashed · 2015-02-07T11:48:50Z

This may require to rename change event to update to be semantically consistent.

dashed · 2015-02-07T12:01:08Z

src/structure.js

+    unsubscribeAll: function () {
+      cursor = void 0;
+      if (!self._pathListeners[pathId]) {
+        referenceListeners = [];


should be referenceListeners = [changeListener];

~~Nevermind.. ignore above comment.~~

See: https://github.com/omniscientjs/immstruct/pull/30/files#r24289215

dashed · 2015-02-07T12:52:40Z

We'll need a way to completely destroy "everlasting cursors". It'll probably require a dead man's swtich variable, to make all methods of the reference cursor to be in noop mode.

We need this, because self._pathListeners[pathId] will become polluted with changeListener functions, even if the reference cursor is GC'd.

dashed · 2015-02-07T13:26:17Z

src/structure.js

+      referenceListeners = void 0;
+      cursor = void 0;
+
+      this.onChange = void 0;


Hmm. This is alright; but not solid. If, for some reason, someone assign a method to some variable, then the method can still be called. It might be solid enough to have if (dead) return; at the top of each method.

dashed · 2015-02-07T13:31:09Z

Throwing this idea in.

Since there is a cursor() method, I'm wondering if it's equally useful to have structure() method to get the structure object that's associated with the cursor/reference?

dashed · 2015-02-07T13:39:24Z

src/structure.js

@@ -68,6 +77,54 @@ Structure.prototype.cursor = function (path) {
  return Cursor.from(self.current, path, changeListener);
 };

+Structure.prototype.reference = function (path) {
+  var self = this, pathId = pathString(path);
+  var listenerNs = self._pathListeners[pathId];


I'm wondering there should be another dimension to self._pathListeners[pathId]. One for reference cursors.

Example:
self._pathListeners[pathId][referenceKey][funIdx]

I think the current solution is a better approach in terms of iterating over and executing listeners on swap.

Hmm.. I guess it's fine when self._pathListeners[pathId] isn't hugely packed.

I just naturally think of big O cases 😄

I don't think the list will be too long. Maybe we need to see the use cases and revise this, but as for now, I think this works just as good.

dashed · 2015-02-07T14:24:40Z

~~Putting here for posterity: Suggesting to rename to Structure.observe(keyPath) which returns an "observer" object.~~

See: https://gitter.im/omniscientjs/omniscient?at=54d621c1c4386fab2709bf4c

dashed · 2015-02-07T14:42:00Z

src/structure.js

+        self._pathListeners[pathId].splice(fnIndex, 1);
+      };
+    },
+    cursor: function () {


What if we want to get sub-cursor via an everlasting cursor?

cursor: function (keyPath) { return cursor(keyPath); },

I started something similar yesterday, but it wasnt that straight forward. You need to alter listener location (reference listener)

I don't follow. I'm thinking that these are equivalent

everlasting.cursor(keyPath)

everlasting.cursor().cursor(keyPath)

With the patch I just committed, this will work better: 7333413

Problem, as you see, is that the path lookup didn't allow for sub-cursors.

skevy · 2015-02-07T15:51:06Z

This is awesome that this is being worked on!

The only thing I would say with this is that the API for onChange is odd, simply because the structure is an EventEmitter, with .on. So I'm wondering if we also make the reference an EventEmitter, so that it the API between a structure and a reference is similar?

dashed · 2015-02-07T15:58:40Z

I'm throwing around this idea: https://gitter.im/omniscientjs/omniscient?at=54d6273cdb625d410e7c26bf

Usage story:

Structure.reference(keyPath) would return an "everlasting cursor".
And on this everlasting cursor, we can observer/unobserve on it.
And destroy it.
onChange and unsubscribeAll is renamed to observe and unobserveAll respectively.
.observe(event, listener) and .observer(listener) where event is either add, delete, change, or swap.
.observe(listener) is shorthand for .observe('swap', listener)

skevy · 2015-02-07T16:11:44Z

@dashed you don't think using an EventEmitter and .on instead of .observe would be better? I was just thinking it'd be good to keep all the api's the same between the structure and the reference.

dashed · 2015-02-07T16:22:10Z

@skevy An EventEmitter is already used indirectly here:

immstruct/src/structure.js

Lines 39 to 44 in 8d7eba1

    
           this.on('swap', function (newData, oldData, keyPath) { 
        
             (self._pathListeners[pathString(keyPath)] || []).forEach(function (fn) { 
        
               if (typeof fn !== 'function') return; 
        
               fn(newData, oldData, keyPath); 
        
             }); 
        
           });

To elaborate, Structure.reference(keyPath) is a more abstract concept wherein it creates and returns an everlasting cursor. An everlasting cursor holds a cursor of the same keyPath internally, and keeps that cursor fresh every time value at keyPath changes. Traditionally, cursors would become stale.

We add features to an everlasting cursor; such as observation. Through the everlasting cursor, we can 'observe' the internal cursor. That's why .on semantics (EventEmitter) isn't really used.

dashed · 2015-02-07T16:42:31Z

I was just thinking it'd be good to keep all the api's the same between the structure and the reference.

This may be a good argument against .observe(event, listener).

At the moment, onChange is way too low-level, in that it's pretty much equivalent to:

structure.on('swap', (newData, oldData ,path) => {
  if(!equalPath(path, keyPath))
    return;

  // ...
});

Thereby making the reference cursor dance a little silly.

dashed · 2015-02-07T16:45:20Z

Idea: Given a cursor, without an official public API to access the associated keyPath, we'd like to convert this into a reference cursor.

mikaelbr · 2015-02-08T14:05:13Z

I'm thinking this is getting mature enough to squash and merge? Any objections, folks?

skevy · 2015-02-08T14:58:33Z

Looks good to me!

dashed · 2015-02-08T15:21:00Z

Looks fine to me. I think later on, we should explore .on() or similar.

mikaelbr · 2015-02-08T15:23:03Z

At some point we can add specific evented listeners (add, change, delete). I have some stashed code for it, but for now it's not critical for this change. This is something we potentially can build on.

…sors) Adds possibility to "scope" down on a given path, and have a reference to a given cursor which always returns the updated cursor. Also allows for adding specified listeners for different paths: `structure.reference(['some', 'path']).observe(function);`

Adds support for "reference cursors"

skevy · 2015-02-08T15:42:52Z

Awesome job @mikaelbr.

mikaelbr · 2015-02-08T15:43:36Z

Thanks! 🍰

skevy · 2015-02-08T15:43:38Z

Now we just need docs :)

mikaelbr · 2015-02-08T15:44:10Z

Indeed. Was just having the discussion with @torgeir . I'm going to open issues.

skevy · 2015-02-08T15:44:39Z

K.

torgeir mentioned this pull request Feb 5, 2015

Any plans to support Om-like reference cursors? omniscientjs/omniscient#42

Closed

dashed reviewed Feb 5, 2015
View reviewed changes

dashed reviewed Feb 6, 2015
View reviewed changes

dashed reviewed Feb 7, 2015
View reviewed changes

mikaelbr force-pushed the onChange branch from 7c60569 to 340e0ae Compare February 8, 2015 15:24

mikaelbr added a commit that referenced this pull request Feb 8, 2015

Merge pull request #30 from omniscientjs/onChange

9d211da

Adds support for "reference cursors"

mikaelbr merged commit 9d211da into master Feb 8, 2015

This was referenced Feb 8, 2015

Listen for changes to specific paths #18

Closed

Document reference cursors #32

Closed

dashed mentioned this pull request Mar 2, 2015

[FAQ] Listeners immutable-js/immutable-js#357

Closed

mikaelbr deleted the onChange branch March 9, 2015 06:57

.reference() for neverending cursors #30

.reference() for neverending cursors #30

Conversation

torgeir commented Feb 5, 2015

torgeir commented Feb 5, 2015

mikaelbr commented Feb 5, 2015

mikaelbr commented Feb 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashed commented Feb 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torgeir commented Feb 6, 2015

mikaelbr commented Feb 6, 2015

torgeir commented Feb 6, 2015

dashed commented Feb 6, 2015

Choose a reason for hiding this comment

dashed commented Feb 7, 2015

dashed commented Feb 7, 2015

dashed commented Feb 7, 2015

mikaelbr commented Feb 7, 2015

mikaelbr commented Feb 7, 2015

dashed commented Feb 7, 2015

Choose a reason for hiding this comment

dashed commented Feb 7, 2015

dashed commented Feb 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashed commented Feb 7, 2015

Choose a reason for hiding this comment

dashed commented Feb 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashed commented Feb 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skevy commented Feb 7, 2015

dashed commented Feb 7, 2015

skevy commented Feb 7, 2015

dashed commented Feb 7, 2015

dashed commented Feb 7, 2015

dashed commented Feb 7, 2015

mikaelbr commented Feb 8, 2015

skevy commented Feb 8, 2015

dashed commented Feb 8, 2015

mikaelbr commented Feb 8, 2015

skevy commented Feb 8, 2015

mikaelbr commented Feb 8, 2015

skevy commented Feb 8, 2015

mikaelbr commented Feb 8, 2015

skevy commented Feb 8, 2015