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

push transition does not work in ArrayType #75

Closed
cowboyd opened this issue Mar 6, 2018 · 7 comments
Closed

push transition does not work in ArrayType #75

cowboyd opened this issue Mar 6, 2018 · 7 comments
Labels
Projects

Comments

@cowboyd
Copy link
Member

cowboyd commented Mar 6, 2018

It appears that a push for a parameterized array does not work, and the following testcase fails.

describe('pushing onto a parameterized array', function() {
  class Thing {
    description = String;
  }

  it('records both the values and states of the items pushed', function() {
    let array = create([Thing], [])
        .push({description: "weird"})
        .push({description: "behavior"});

    expect(array.valueOf().length).toBe(2);
    expect(array.state.length).toBe(2);
    expect(array.state[0]).toBeInstanceOf(Thing);
    expect(array.state[1]).toBeInstanceOf(Thing);
    expect(array.state[0].description).toEqual("weird");
    expect(array.state[1].description).toEqual("behavior");
  });
});

The valueOf() only contains the last thing pushed on:

 Expected value to equal:
      [{"description": "weird"}, {"description": "behavior"}]
    Received:
      [{"description": "behavior"}]

Furthermore, the state property completely fails insofar as it is equal to []

@cowboyd cowboyd changed the title Push transition does not work in ArrayType push transition does not work in ArrayType Mar 6, 2018
@cowboyd
Copy link
Member Author

cowboyd commented Mar 9, 2018

Among other things, it appears that the problem is that we don't allow transitions returning just the value to update the tree.

https://github.com/cowboyd/microstates.js/blob/fix-array-transitions/src/typeclasses.js#L13

As you can see there, we update the value, but not the tree. However, in the case of Arrays and Objects the tree can update with a transition as well. It seems as though we need to do some sort of "re analysis".

@cowboyd
Copy link
Member Author

cowboyd commented Mar 9, 2018

So for example let's say you're pushing 4 onto the array [1,2,3] using ArrayType#push() https://github.com/cowboyd/microstates.js/blob/fix-array-transitions/src/types/array.js#L12-L14

The value of the array node will get updated to have [1,2,3,4], but the tree will still only have 3 children.

@taras
Copy link
Member

taras commented Mar 9, 2018

@cowboyd we must not have compounded transition tests for arrays, because our test suite should have caught this.

@cowboyd
Copy link
Member Author

cowboyd commented Mar 9, 2018

Our existing tests couldn't have caught it because you don't see this problem until you add parameterized types because for non-parameterized arrays, there is no tree below the array... The array itself is a leaf, so the value is the state. However, once you start parameterizing, then the tree can extend beyond the array node, and so it's important to keep the tree in sync.

@cowboyd
Copy link
Member Author

cowboyd commented Mar 12, 2018

One way to brute-force it, would be to have array transitions that change the structure of the array actually return a completely new microstate, and not just a new value. However in order to create a new Array with the correct type, we need to somehow be able to pass the type of the current array to the transition so that it can create the correct type. One way to do this would be to have all array transitions return a function that takes the type of the array, and the current value of the array, so that it can return a new microstate instance. That way, it will use the entire microstate tree returned.

class ArrayType {
  pop() {
    return (T, value) => create(T, value.slice(0, -1));
  }
}

There are two problems with this. One is that it doesn't do any structural sharing between versions of the array. For example, if I have an array [1,2,3] and then I pop off 3, then the trees for [1,2] should not need to be created from scratch. Only the tree representing the array changes to indicate that 3 is no longer a child.

The other problem that arises is how do we type-shift? In other words, how can we have a heterogeneous set of types occupying an array if popping off an item, resets the type to the original parameter. In other words, If I have an array of Session objects, [Session, Authenticated, Authenticated, Anonymous] then we want popping to preserve the type at the individual index.

We could define pop in terms of a slice transition:

class ArrayType {
  pop() {
    //kick the can down to hypothetical slice transition
    return this.slice(0, -1);
  }
  slice(start, end) {
    let { tree, value } = reveal(this);
    let nextTree = new Tree({
      data: _ => tree.data,
      children: _ => tree.children.slice(start, end)
    });
    let nextValue = value.slice(start, end);
    return new Microstate(nextTree, nextValue);
  }

That is a solution for pop(), where we're ejecting an old value and don't really care what type it was, but what about when we're closing over a new value. We need to have the type of the array. Something like

class ArrayType {
  push(value) {
    let index = this.valueOf().length;
    let { tree, value } = reveal(this);
    let { T }  = params(tree.data.Type);
    let { tree: pushedTree } = reveal(create(T, value));
    let nextTree = new Tree({
      data: _ => tree.data,
      children: _ => tree.children.concat(graft([index], pushedTree))
    });
    let nextValue = value.concat(value);
  }

yikes! How to abstract this?

🤔

@taras taras added this to To do in Roadmap Mar 19, 2018
@Robdel12 Robdel12 added the bug label Mar 21, 2018
@cowboyd cowboyd moved this from To do to Done in Roadmap Apr 24, 2018
@taras
Copy link
Member

taras commented May 13, 2018

This issue is addressed by the introduction of Structural Sharing in #113. It is now possible to perform operations on the tree that the microstate is created from. To do this, you can map the microstate and flatMap the tree.

You can see this in the Array & Object types https://github.com/microstates/microstates.js/blob/cl/a-tree-for-all-seasons/src/types/array.js#L52

@cowboyd
Copy link
Member Author

cowboyd commented Jul 29, 2018

This has been crushed in like 3 re-writes :)

@cowboyd cowboyd closed this as completed Jul 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Roadmap
  
Done
Development

No branches or pull requests

3 participants