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

Observe Events on forceHasSwapped #73

Open
blittle opened this issue Jun 10, 2015 · 8 comments
Open

Observe Events on forceHasSwapped #73

blittle opened this issue Jun 10, 2015 · 8 comments

Comments

@blittle
Copy link

blittle commented Jun 10, 2015

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.

blittle added a commit to blittle/immstruct that referenced this issue Jun 10, 2015
@dashed
Copy link
Contributor

dashed commented Jun 11, 2015

That observer wouldn't be called unless there's a true change. It's not being call because of this line:

if (!map || newData === oldData) return;

Your failing test at blittle@a8c1f0f is correct behaviour.

@blittle
Copy link
Author

blittle commented Jun 11, 2015

@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
Copy link
Contributor

dashed commented Jun 11, 2015

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 #60

@mikaelbr
Copy link
Member

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
Copy link
Author

blittle commented Jun 11, 2015

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

@dashed
Copy link
Contributor

dashed commented Jun 12, 2015

@blittle Yeah, it was something I wanted as well. I originally reported this in #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
Copy link
Member

IMO, master seems pretty stable.

I consider it stable as well. One thing I have in mind is this #60 (comment). 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
Copy link
Contributor

dashed commented Jun 12, 2015

Ah yeah. I found #60 (comment) 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?

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

3 participants