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

references does not work at all (or aren't so powerful as I was expecting). #57

Closed facundocabrera closed 9 years ago

facundocabrera commented 9 years ago

I'm trying to understand this behavior:

'use strict';

var ims = require('immstruct'),
  im = require('immutable');

var ds = ims();

// initialize somewhere in the code
ds.cursor().update(function() {
  return im.fromJS({
    k: 'hello world'
  });
});

// create a reference somewhere else
var ref = ds.reference('k');

// and finally update the structure in some way
ds.cursor().update(function() {
  return im.fromJS({
    k: {
      value: 10
    }
  });
});

// print the result
console.log(ref.cursor().deref());

But, if I'm specific enough about the key I'm interested to change:

'use strict';

var ims = require('immstruct'),
  im = require('immutable');

var ds = ims();

ds.cursor().update(function() {
  return im.fromJS({
    k: 'hello world'
  });
});

var ref = ds.reference('k');

ds.cursor('k').update(function(oldValue) {
  return 10;
});

console.log(ref.cursor().deref());

Seems it works perfectly, so could be this behavior considered a bug? Because, at the moment I create the references, I don't know the state of the immutable data structure, so I can't be as specific as the API requires to make my code works.

Thanks in advance

mikaelbr commented 9 years ago

Hi and thanks for your issue!

For now this isn't implemented. The problem is that with the current implementation of cursors on the Immutable.js library, you won't get the path to the updated part of the structure by doing it the way you do in example 1.

We would see this by printing the path on update:

ds.on('swap', function (a, b, path) {
  console.log(path);
});
// Prints
//=> []
//=> []

So there is no clear way to get what sub-path that was updated, without traversing the updated sub-structure and manually triggering every listener. This might be possible, but it might also be costly in terms of performance if you rely on many listeners/references.

facundocabrera commented 9 years ago

I completely understand the issue, but the documentation presents references in a way I was almost crying of happiness :smile:

Thanks for your answer, and if you want close the ticket as "won't fix" just go ahead, for now I can't contrib with a solution, but maybe someone else have time to propose something.

mikaelbr commented 9 years ago

I think we have the possibility of making them this powerful, but it would probably come at a larger high cost, performance-wise. But it is worth checking out / getting some real numbers for whether or not it's feasible.

andrewluetgers commented 9 years ago

@dendril I have dealt with this by doing a diff on the new and old state objects from swap, you can check the diff for the existence of the path you are interested in.

dashed commented 9 years ago

This is related to https://github.com/omniscientjs/immstruct/issues/37.

mikaelbr commented 9 years ago

And also #60

mikaelbr commented 9 years ago

@dendril This works now, no?

facundocabrera commented 9 years ago

I'll run the test ASAP with my current project so I can get some real feedback

mikaelbr commented 9 years ago

@dendril How did this go? I added a test from your example above: https://github.com/omniscientjs/immstruct/blob/e1d2488c3d0791fc212494beffcf3601b67d8443/tests/structure_test.js#L735-L780

facundocabrera commented 9 years ago

@mikaelbr sorry for the delay, looks it works I was expecting :smile: I have only 1 test pending combining history undo/redo and references. From my side, this issue is fixed.

Thanks to everyone!

mikaelbr commented 9 years ago

Good! I'm closing this then.