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

Private to[Member] member not properly setting state #9

Open
TeknoFiend opened this issue Apr 19, 2019 · 5 comments
Open

Private to[Member] member not properly setting state #9

TeknoFiend opened this issue Apr 19, 2019 · 5 comments
Labels
bug Something isn't working

Comments

@TeknoFiend
Copy link

The example in the README wasn't working for me so I modified it a bit to extract some info about what is happening. I believe I'm doing this correctly.

The initial state value is "foo". After the resolve call the state value appears to be a String type representation.

const PromiseType = Union({
  Pending: PromiseType => class extends PromiseType {
    resolve(result) {
      return this.toResolved(result);
    }
  },
  Resolved: PromiseType => class extends PromiseType {},
});

let p = PromiseType.Pending.create("foo");

console.log( p.isPending, p.isResolved );  // =>  true false
console.log( p.state );   // =>  foo

p = p.resolve("great!");

console.log( p.isPending, p.isResolved ); // => false true
console.log( p.state );   // =>  
  //  String {length: 3}
  //    0: (...)
  //    1: (...)
  //    2: (...)
  //    3: (...)
  //    4: (...)
  //    length: 3
  //    get 0: ƒ ()
  //    get 1: ƒ ()
  //    get 2: ƒ ()
  //    get 3: ƒ ()
  //    get 4: ƒ ()
  //    __proto__: String

I hope this is helpful.

@cowboyd
Copy link
Member

cowboyd commented Apr 19, 2019

I think that the problem is that this really ought to be Any here instead of Object https://github.com/microstates/union/blob/master/src/union.js#L15

Because it's trying to coerce the string into an object which is why you end up with a String() instead of a string.

Looking back, I think we were trying to be a little too cute with the ability to assign to the state

This let you do things like say:

this.toOtherState({merged: properties})

and it won't overwrite, but will instead merge with any of the properties that are stored in the value. In retrospect, I think this was too clever by half.

@TeknoFiend
Copy link
Author

Yeah the merging would be awesome. I want to use an object, actually, and was hoping it would do that (seems natural). I just tried with an object and ran into a couple more issues, I can open separate tickets if you like. I tried to make a PR but couldn't get to "working"...

I think this line should be let value = valueOf(this.value) : https://github.com/microstates/union/blob/master/src/union.js#L67

And I think the first two conditions (in the next 4 lines) are backwards (I think we want assign if it's an object, right?).

Even after making those changes, tho, I wasn't getting a merge or even my new value after the state transition so there must be something else I'm missing.

@cowboyd cowboyd added bug Something isn't working and removed triage labels Apr 20, 2019
@cowboyd
Copy link
Member

cowboyd commented Apr 20, 2019

I think you're right. Here's the thing though, you can always override the type of the value, in your actual union.

class Base {
  value = Merger;
}

export default Union({Pending: Base => class Pending extends Base {}}, Base);

So maybe it makes sense to just make it Any and keep it to a simple set?

@TeknoFiend
Copy link
Author

Yes I see what you mean 👍

@cowboyd
Copy link
Member

cowboyd commented Apr 20, 2019

Good news is, if it's doesn't work, well we'll trying something else :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants