omniscientjs / immstruct

Immutable data structures with history for top-to-bottom properties in component based libraries like React. Based on Immutable.js
374 stars 21 forks source link

Test out "RC" in master #76

Closed mikaelbr closed 9 years ago

mikaelbr commented 9 years ago

I think we're ready to unleash the beast that is Immstruct 2.0.0 on to the world. A draft of the changelog can be seen in this gist.

Would appreciate someone other than me doing a QA/proof on this before we publish it. Verifying that it works as expected.

Changes in this release includes terrific contributions from people like @jeffbski, @andrewluetgers, @tomasd, @rufman, @amccloud and @frederickfogerty. And of course, by great help as always by @Dashed.

mikaelbr commented 9 years ago

Before release, we need to look more into https://github.com/omniscientjs/immstruct/pull/60#discussion_r29989545

Summarized from Gitter: Maybe the best approach is the remove the keyPath from references as they are kind of implicit through the reference key path provided as an argument. Need to revise and think about what arguments should be passed to observers for references.

dashed commented 9 years ago

Mentioned in gitter:

Looking at all the events, I think reference events should adopt swap-like semantics

When doing something like: struct.on('swap', listener). listener has signature (newData, oldData, pathToChange). newData, oldData are root.

But if you do ref.observe(listener2), listener2 may have signature (newValue, oldValue, pathChangeOrigin). newValue and oldvalue are relative to ref path. and pathChangeOrigin is the path where the change originated. Very similar semantics to swap.

dashed commented 9 years ago

For posterity: https://gitter.im/omniscientjs/omniscient?at=558b084738e37bf74261c7c6

I think it should be clarified that 'swap' can capture any other event such as delete, update, and add. Folks may not be familiar with how onChange in Cursor.from is used internally.

mikaelbr commented 9 years ago

Very true. Updated now in the README and in the Api Reference. I've also updated the drafted changelog

mikaelbr commented 9 years ago

From gitter:

With the new reference cursors, we'd expect this to work:

var structure = immstruct({ 'foo': 'bar' });
var ref = structure.reference(['foo', 'bar']);
ref.observe('add', function (newValue) {
    // this function is not called
});
structure.cursor(['foo']).update(function () { return Immutable.fromJS({'bar': 'qux'}); });
mikaelbr commented 9 years ago

Thinking about it, the code above should actually not work. As we update the structure placed in the path ['foo'] and something at ['foo'] exists, this is a change event, not a add event.

var structure = immstruct({ 'foo': 'bar' });
var ref = structure.reference(['foo', 'bar']);
ref.observe('change', function (newValue) {
  // this function *is* called
});
structure.cursor(['foo']).update(function () { return Immutable.fromJS({'bar': 'qux'}); });
dashed commented 9 years ago

But the reference cursor points to ['foo', 'bar']. Not ['foo'].

mikaelbr commented 9 years ago

But the changed cursor is ['foo']. So the actually changed value is ['foo'], no?

dashed commented 9 years ago

Yes the changed cursor is ['foo']. The new value at ['foo'] is a new immutable collection.

So, now you propagate these changes to all subpaths of ['foo'] (ancestors) and superpaths of ['foo'] (descendents). Since value at ['foo'] is an immutable collection, it's traversed in case there are any listeners at any superpath that exists.

Wasn't this a new behavior that's added for v2?https://github.com/omniscientjs/immstruct/blob/5bdb2631ab343863895d35565728dfb9f4a4d58b/src/structure.js#L143-L163

dashed commented 9 years ago

To clarify, the value at ['foo'] was changed from a non-immutable collection to an immutable collection.

mikaelbr commented 9 years ago

Wasn't this a new behavior that's added for v2?https://github.com/omniscientjs/immstruct/blob/5bdb2631ab343863895d35565728dfb9f4a4d58b/src/structure.js#L143-L163

Yeah, but now the it's more a issue of "speccing" it. What should be the correct behaviour of cases like this. Is it a add or is it a change? Should it have root with the path of the cursor that changed it, or the root with the path that the reference points at.

dashed commented 9 years ago

I assumed it's the path that the reference points at.

My reasoning is that if I have a reference cursor at path keyPath, and I'd like to observe this reference cursor with listener whenever some event occurs at keyPath:

var ref = structure.reference(keyPath);
ref.observe(event, listener);

The API contract of doing ref.observe stipulates that the values passed to listener should be with respect to the path associated with the reference cursor (i.e. keypath) and only at this path. In this manner, we can make listener function to be pure.


Did you have another behaviour in mind?

mikaelbr commented 9 years ago

As it is now, swap events give structures that root in the current structure (structure.current) and for references, where the reference is created. But the semantics are different for events like add, change and remove. For these events, the passed argument is the actual changed value. So the value calculated from the cursor used to change the underlying structure.

You might sum it up for swap is general, specific events are specific.

dashed commented 9 years ago

As it is now, swap events give structures that root in the current structure (structure.current) ...

If this is the case, I think there should also be an event to capture any events (e.g. add, change and remove) and receive the changed value. Maybe the 'any' event?

mikaelbr commented 9 years ago

any with the same semantics as add, change and delete (not remove, sorry)? That might make sense.

dashed commented 9 years ago

Yeah. I think we really need an any event to capture values. I was treating swap as an any event:

structure.on('swap', function(newData, oldData, path) {
    anyListener.call(null, newData.getIn(path), oldData.getIn(path), path);
});

structure.on('any', anyListener);
mikaelbr commented 9 years ago

I'll look into it.

dashed commented 9 years ago

Did a litmus usage test on master and noticed the following:

const Immutable = require('immutable');
const immstruct = require('./');

var structure = immstruct({ foo: { bar: null } });
var ref = structure.reference(['foo', 'bar', 'baz']);

['swap', 'any', 'delete', 'add', 'change'].forEach(function(event) {

    ref.observe(event, function (newValue, oldValue, path) {

        console.log('event', event);
        console.log('path', path);
        console.log('newValue', newValue);
        console.log('oldValue', oldValue);
        console.log('---');
    });

});

structure.cursor(['foo', 'bar']).update(function() {
    return Immutable.fromJS({
        baz: 'baz'
    });
});

// output:
//
// event swap
// path []
// newValue baz
// oldValue undefined
// ---
// event any
// path [ 'foo', 'bar' ]
// newValue Map { "baz": "baz" }
// oldValue null
// ---
// event change
// path [ 'foo', 'bar' ]
// newValue Map { "baz": "baz" }
// oldValue null
// ---

Shouldn't the 'add' event be emitted?

mikaelbr commented 9 years ago

Hm. Probably. It uses the path for the cursor used to manipulate the data, so I get why it isn't triggered, but perhaps it should be.

mikaelbr commented 9 years ago

I think this can be a limitation for now, and do a minor release on a later point.

mikaelbr commented 9 years ago
➜  immstruct git:(master) npm publish
+ immstruct@2.0.0
jprichardson commented 9 years ago

➜ immstruct git:(master) npm publish

  • immstruct@2.0.0

:+1: yay.

You may want to fix the readme (the note about unreleased version 2.0). :)

mikaelbr commented 9 years ago

Good catch!