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

[TypeScript] Assigning a simple object to a map property #913

Closed
pelotom opened this issue Jul 9, 2018 · 12 comments
Closed

[TypeScript] Assigning a simple object to a map property #913

pelotom opened this issue Jul 9, 2018 · 12 comments

Comments

@pelotom
Copy link

pelotom commented Jul 9, 2018

In TypeScript, it doesn't seem easy to assign to a map prop in a type safe way. For example,

const Foo = types
  .model({
    m: types.maybe(types.map(types.string))
  })
  .actions(self => ({
    initialize() {
      self.m = {
        x: "hello",
        y: "world"
      } as any; // <-- or else we have a type error
    }
  }));

To work around it I would've thought I could write

      self.m = types.map(types.string).create({
        x: "hello",
        y: "world"
      });

but this causes a runtime error

[mobx-state-tree] Error while converting @(2 items)> to `(map | null)`:

    value of type map: @(2 items)> is not assignable to type: `(map | null)`, expected an instance of `(map | null)` or a snapshot like `(Map | null?)` instead. (Note that a snapshot of the provided value is compatible with the targeted type)
@srolel
Copy link

srolel commented Jul 10, 2018

The problem is that when you call types.map it returns a distinct type. So these are 2 different types that happen to describe the same thing:

const map1 = types.map(type.string);
const map2 = types.map(type.string);

Because it checks for equality of the type by reference, the MST type system doesn't allow this, for example:

const map1 = types.map(types.string);
const map2 = types.map(types.string);

const Foo = types
  .model({
    m: types.maybe(map1)
  })
  .actions(self => ({
    initialize() {
      self.m = map2.create({
        x: "hello",
        y: "world"
      });
    }
  }));

new Foo().initialize();

However, if you did this:

const map = types.map(types.string);

const Foo = types
  .model({
    m: types.maybe(map)
  })
  .actions(self => ({
    initialize() {
      self.m = map.create({
        x: "hello",
        y: "world"
      });
    }
  }));

new Foo().initialize();

It would work because the types are compatible in that case.

@mattiamanzati
Copy link
Contributor

Not sure, but ideally if we could do #893 we could fix this in the future

@pelotom
Copy link
Author

pelotom commented Jul 10, 2018

@mosho1 thanks, that's very helpful... I think this explains another issue I was seeing. This is somewhat unexpected and disappointing, I had assumed because MST types were immutable that they also had value semantics, so that if you build up a type the same way on 2 separate occasions, then values created by them will be compatible. I guess not 😕

@Bnaya
Copy link
Member

Bnaya commented Jul 10, 2018

MST 3 will allow typesafe applySnapshot (But not assigns)

Nowdays, applySnapshot takes any snapshot,
on MST 3, using conditional types its possible to extract real snapshot interface from the model declaration, and applySnapshot, and all of the other snapshot handling methods will be typesafe

    initialize() {
      applySnapshot(self.m, {
        x: "hello",
        y: "world"
      }
    })

@pelotom
Copy link
Author

pelotom commented Jul 10, 2018

Out of curiosity, why is assignment from instances of compatible models not allowed? I would think that an instanceof check would only be used as a shortcut to approval, but not sufficient to cause an error. If the instanceof check fails then you would fall back to attempting to treat the object as a snapshot... am I way off base here?

@Bnaya
Copy link
Member

Bnaya commented Jul 10, 2018

You can't attach living model (which is his own tree) to another tree.
You you can detach part of a tree but you can't attach. you have to assign/apply plain snapshot

@pelotom
Copy link
Author

pelotom commented Jul 10, 2018

@Bnaya ah, I see... that's a different issue than what I was thinking of then.

@mweststrate
Copy link
Member

mweststrate commented Jul 12, 2018

Out of curiosity, why is assignment from instances of compatible models not allowed?

You cannot safely assume one type of model with a similar signuature to be compatible with the other?

E.g. are these types compatible: types.model({ x: types.refinement(types.number, x => x > 5) and types.model({ x: types.refinement(types.number, x => x > 6)? They are not compatible, as their domains of instances don't match, but how could MST know unless by trying all possible values for x?

Or what if the properties match, but the action / view signatures don't? This is the same problem as why declaring two javascript classes are not instanceof compatible, while they might structurally be the same. But whether the types are fully compatible actually depends on the implementation of their methods! And then it still depends whether you need generic lower or upperbounds....

So you can't interchange model types in MST, but you can interchange their snapshots, which is usually good enough, and which is what you were doing initially. The reason that that can't pass the type check is because TS cannot distinguish between the 'read' and 'write' type of a property. However, that is something we can't change and is a TS limitation. E.g. the following doesn't work either, as TS will complain that getter and setter must have the same type:

class X {
   this._value: number
   get attribute(): number {
      return this._value
   } 
   set attribute(v: number | string) {
        if (typeof v === "string")
            this._value = parseInt(v)
        else this._value = v
   }
}

@pelotom
Copy link
Author

pelotom commented Jul 12, 2018

@mweststrate all of what you said are valid reasons why you can't simply treat one instance as an instance of another class... my point was this:

If the instanceof check fails then you would fall back to attempting to treat the object as a snapshot

What is the downside of attempting to treat an instance of a different class as a snapshot? If it succeeds structurally as a snapshot, that's good enough.

@mweststrate
Copy link
Member

mweststrate commented Jul 13, 2018

@pelotom

suppose you have a tree like

X
  a: maybe(Z)
  b: maybe(Z)
  c: maybe(SomethingLikeZ)

the behavior would be inconsistent if the snapshotting was done automatic

x.b = x.a // moves the thing in a to b
x.a // null
x.b // same thing as was in x.a

hooks triggered: beforeDetach, afterAttach

vs.

x.c = x.a // not compatible, so clones, so no move (or suddent death of what was in x.a)
x.a // ???
x.c // clone of what was in x.a

hooks triggered: beforeDestroy? afterCreate, afterAttach

@pelotom
Copy link
Author

pelotom commented Jul 13, 2018

@mweststrate I see, thanks for the illustrating examples, that's very helpful. For context, I was trying to make a more type safe alternative to getEnv, by simply making the model a function of the environment:

const Foo = (env: Env) => types
  .model({ ... })
  .views(self => ({ ... }))
  .actions(self => ({ ... }));

const foo = Foo(env).create({ ... });

but it didn't work if I needed to construct the model from the env in different places. I suppose this could be solved by simply memoizing the model function, since the env argument will always be the same in a given run of the app.

@mweststrate
Copy link
Member

mweststrate commented Jul 13, 2018 via email

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

5 participants