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

Observe Events on forceHasSwapped #73

Open blittle opened 9 years ago

blittle commented 9 years ago

Should calling forceHasSwapped forcibly trigger a swap event for observing reference cursors?

For example:

const ref = structure.reference(['data']);
ref.observe('swap', () => {
  //should this be called?
});

const old = structure.current;
structure.forceHasSwapped(structure.undo(1), old);

Currently it does not appear as though it is getting called.

dashed commented 9 years ago

That observer wouldn't be called unless there's a true change. It's not being call because of this line: https://github.com/omniscientjs/immstruct/blob/d22d54cd6a0c78a2c41f40f1410d46bc93c49f60/src/structure.js#L98

Your failing test at https://github.com/blittle/immstruct/commit/a8c1f0fe18d6ef7cd52f5138e17862c43c500f4b is correct behaviour.

blittle commented 9 years ago

@Dashed so I think my problem is the version that is published on NPM seems substantially different than the master branch here. When I run off of master, I don't have any problems.

dashed commented 9 years ago

Ah yeah. v1.4.1 doesn't have the overhauled event propagation for reference cursors: https://github.com/omniscientjs/immstruct/blob/v1.4.1/src/structure.js

Overhaul came from here https://github.com/omniscientjs/immstruct/pull/60

mikaelbr commented 9 years ago

Yeah, I've marked the 2.0.0 for release (https://github.com/omniscientjs/immstruct/milestones/2.0.0). Hoping to get it out within the month. I'm trying out different things with Omniscient.js and have done some Omniscient rewrites that I've later thrown away.

blittle commented 9 years ago

@mikaelbr @Dashed excited to see it released! Until then, reference cursors are rather broken :(

dashed commented 9 years ago

@blittle Yeah, it was something I wanted as well. I originally reported this in https://github.com/omniscientjs/immstruct/issues/37

IMO, master seems pretty stable.


Offtopic: @mikaelbr Is this the branch you're working on for omniscient? https://github.com/omniscientjs/omniscient/tree/next

mikaelbr commented 9 years ago

IMO, master seems pretty stable.

I consider it stable as well. One thing I have in mind is this https://github.com/omniscientjs/immstruct/pull/60#discussion_r29989545. Only reason I haven't published a new release is to have into consideration if the API should change if there is something we discover during the Omniscient fixes - but as for now, I don't predict any massive changes in Omniscient. Maybe not even a major version bump.

Offtopic: @mikaelbr Is this the branch you're working on for omniscient? https://github.com/omniscientjs/omniscient/tree/next

Originally, yes. But that is for testing with .equals, which we uncovered was rather slow. So I have a local branch where I'm trying to do some simplifications using valueOf instead of deref. But thus far it seems there aren't any real simplifications as we still would need to know if it is cursor/immutable structure if we want to be able to support multiple cursor/immutable-implementations.

dashed commented 9 years ago

Ah yeah. I found https://github.com/omniscientjs/immstruct/pull/60#discussion_r29989545 to be quite important via a custom cursor implementation. Since you have knowledge of the keypaths (through ref cursors), you should only care about what changed in those keypaths.


If you really wanted to, you can wrap to mimic the .equals API for cursors:

const cursorCompare = curry(cursorCompare);

const makeEquals = function(obj) {
    const curried = cursorCompare(obj);
    return {
        equals(obj2) {
            return curried(obj2);
        }
    };
};

That way you can maintain whatever patterns you're trying to achieve, no?