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

Idea: writeable(instance) #955

Closed
3 tasks done
xaviergonz opened this issue Jul 31, 2018 · 13 comments
Closed
3 tasks done

Idea: writeable(instance) #955

xaviergonz opened this issue Jul 31, 2018 · 13 comments
Assignees
Labels
brainstorming/wild idea Brainstorming / Wild idea has PR A Pull Request to fix the issue is available

Comments

@xaviergonz
Copy link
Contributor

xaviergonz commented Jul 31, 2018

I have:

  • Feature request
    • Describe the feature and why you feel your feature needs to be generically solved inside MST
    • Are you willing to (attempt) a PR?

Not following the above template might result in your issue being closed without further notice

wild idea,

given

const A = types.model({n: 123})
          .actions((self) => ({setN(n: number) {self.n = n}}))
const M = types.model({a: A})

as you know this fails:

model.a = {} // fails because it has to be instance of A

// have to do this instead, which looses helpful errors on the snapshot
model.a = {} as typeof A.Type 

// or this to get a bit more help
const aSnapshot: typeof A.CreationType = {}
model.a = aSnapshot as typeof A.Type

given that TS does not accept different types for getting/setting values on models/arrays

so, how about something like this?

IDEA 1

// for models
model.a = {} // fails because it has to be instance of A
writeable(model).a = {} // ok, creation type or instance type of model A

// for arrays
arrayOfA[0] = {} // fails
writeable(arrayOfA)[0] = {} // ok

// for maps
mapOfA.set('key', {}) // fails? type could be fixed to make the setter accept Creation or Instance of A
writeable(mapOfA).set('key, {})) // would work

// if many write ops are required
const wModel = writeable(model)
wModel.a = {}
// however wModel.a.action would not work

// to make it shorter writeable could be imported as whatever, e.g.
import { writeable as $ } from "mobx-state-tree"
$(model).a = {}

Requirements:

  • types would need a new subtype besides C,S,T, namely a WT, which is like T but accepting creation|instance type for members
  • the writeable function would be a dummy function that just casts from T to WT and returns the same object

IDEA 2
personally I don't like it as much as IDEA 1 though

set(model, 'a', {})
set(arrayOfA, 0, {})
set(mapOfA, 'a', {})

Requirements:

  • types would need a new subtype besides C,S,T, namely a WT, which is like T but accepting creation|instance type for members
  • the set function would be a dummy function that just calls [key] = a / .set(key, a) internally

IDEA 3
a dummy provided function that given type will ensure that the input is a proper snapshot and "output" the instance type
in reality the function won't do anything

// OPTION A
model.a = asInstance<typeof A>({})
arrayOfA[0] = asInstance<typeof A>({})
mapOfA.set('a', asInstance<typeof A>({}))

// OPTION B
model.a = asInstanceOf(A, {}) // castTo(A, {}) ?
arrayOfA[0] = asInstanceOf(A, {})
mapOfA.set('a', asInstanceOf(A, {}))

// OPTION C
model.a = A.cast({}) // .castSnapshot? .asInstance?
arrayOfA[0] = A.cast({})
mapOfA.set('a', A.cast({}))

Requirements:

  • just this
// OPTION A
function asInstance<IT extends IType>(snapshotOrInstance: ExtractC<IT> | ExtractT<IT>): ExtractT<IT> { return snapshotOrInstance; }

// OPTION B
function asInstanceOf<IT extends IType>(type: IT, snapshotOrInstance: ExtractC<IT> | ExtractT<IT>): ExtractT<IT> { return snapshotOrInstance; }

// OPTION C
interface IType<C, S, T> {
  ...
  cast(snapshotOrInstance: C | T): T {
    return snapshotOrInstance as any as T; 
  }
}

basically it would be as create, but with the improvement that it doesn't require to specify an environment, since it will just inherit the one from the parent

@k-g-a
Copy link
Member

k-g-a commented Aug 7, 2018

I'd personnaly vote for the IDEA 3

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Aug 7, 2018

@k-g-a from IDEA 3 would you prefer option A, B or C? (I just named them and added a new one)
also, which method name would you prefer?

@k-g-a
Copy link
Member

k-g-a commented Aug 8, 2018

From initial two options I'd prefer generic's one, but typeof part is messy (model = asInstance<A>({}) would be great).

Thrid optins looks attractive. We could call it from(), so it looks like: model.a = A.from({}) - seems semantically perfect to me.

@xaviergonz
Copy link
Contributor Author

sadly asInstance<A>({}) is not possible since A is not a type :-/

I think I also like IDEA 3-C the most, but I'm scared that from might confuse people thinking that it actually generates an instance (like create would) while actually it's still a snapshot (if it was a snapshot) or an instance (if it was an instance).

@mweststrate
Copy link
Member

Great stuff @xaviergonz!

I like Idea 3, option C is the best as well

I agree, A.cast({}) looks the best, and should make it clear we are just tricking the compiler, not actually doing something (like .create would).

@xaviergonz
Copy link
Contributor Author

should I create a PR then?

@mweststrate
Copy link
Member

mweststrate commented Aug 10, 2018 via email

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Aug 10, 2018

@mweststrate @k-g-a
Actually just had an idea 4 :D

// maybe CreationOrInstance?
type SnapshotOrInstance<T> = T extends IStateTreeNode<infer STNC, any> ? STNC | T : T extends IType<infer TC, any, infer TT> ? TC | TT : T;

type CastedType<T> = T extends IStateTreeNode<infer STNC> ? STNC | T : T;
function cast<T = never>(val: CastedType<T>): T {
    return val as any as T;
}

const NumberArray = types.array(types.number)
const NumberMap = types.map(types.number)
const A = types.model({ n: 123, n2: types.number, arr: NumberArray, map: NumberMap }).actions((self) => ({
    // for primitives (although not needed)
    setN(nn: SnapshotOrInstance<typeof self.n>) {
        self.n = cast(nn);
    },
    setN2(nn: SnapshotOrInstance<typeof types.number>) {
        self.n = cast(nn);
    },
    setN3(nn: SnapshotOrInstance<number>) {
        self.n = cast(nn);
    },
    setN4(nn: number) {
        self.n = cast(nn)
    },
    setN5() {
        self.n = cast(5)
    },

    // for arrays
    setArr(nn: SnapshotOrInstance<typeof self.arr>) {
        self.arr = cast(nn);
    },
    setArr2(nn: SnapshotOrInstance<typeof NumberArray>) {
        self.arr = cast(nn)
    },
    setArr3(nn: number[]) {
        self.arr = cast(nn)
    },
    setArr4() {
        // it works even without specifying the target type, magic!
        self.arr = cast([2,3,4])
        self.arr = cast(NumberArray.create([2,3,4]))
    },

    // for maps
    setMap(nn: SnapshotOrInstance<typeof self.map>) {
        self.map = cast(nn);
    },
    setMap2(nn: SnapshotOrInstance<typeof NumberMap>) {
        self.map = cast(nn)
    },
    setMap3(nn: {[k: string]: number}) {
        self.map = cast(nn)
    },
    setMap4() {
        // it works even without specifying the target type, magic!
        self.map = cast({a: 2, b: 3})
        self.map = cast(NumberMap.create({a: 2, b:3}))
    }
}))

const C = types.model({ a: A}).actions((self) => ({
    // for submodels, using typeof self.var
    setA(na: SnapshotOrInstance<typeof self.a>) {
        self.a = cast(na);
    },
    // for submodels, using the type directly
    setA2(na: SnapshotOrInstance<typeof A>) {
        self.a = cast(na);
    },
    // works too
    setA3(na: typeof A.CreationType) {
        self.a = cast(na);
    },
    // works too
    setA4(na: typeof A.Type) {
        self.a = cast(na);
    },
    setA5() {
        // it works even without specifying the target type, magic!
        self.a = cast({n2: 5})
        self.a = cast(A.create({ n2: 5 }))
    }
}))

const c = C.create({a: {n2: 5}})
// all below works
c.setA({ n2: 5 })
c.setA(A.create({ n2: 5 }))
c.setA2({ n2: 5 })
c.setA2(A.create({ n2: 5 }))
c.setA3({ n2: 5 })
// c.setA3(A.create({ n2: 5 })) // this one doesn't work, but that's expected, it wants the creation type
// c.setA4({n2: 5}) // this one doesn't work, but that's expected, it wants the instance type
c.setA4(A.create({ n2: 5 }))
c.setA5()

c.a.setN(1);
c.a.setN2(1)
c.a.setN3(1)
c.a.setN4(1)
c.a.setN5()

c.a.setArr([])
c.a.setArr(NumberArray.create([]))
c.a.setArr2([])
c.a.setArr2(NumberArray.create([]))
c.a.setArr3([])
c.a.setArr3(NumberArray.create([]))
c.a.setArr4()

c.a.setMap({a:2, b:3})
c.a.setMap(NumberMap.create({ a: 2, b: 3 }))
c.a.setMap2({ a: 2, b: 3 })
c.a.setMap2(NumberMap.create({ a: 2, b: 3 }))
c.a.setMap3({ a: 2, b: 3 })
// c.a.setMap3(NumberMap.create({ a: 2, b: 3 })) // doesn't work as expected, wants a plain object
c.a.setMap4()

const arr = types.array(A).create();
arr[0] = cast({n2: 5})

const map = types.map(A).create();
map.set('a', cast({n2: 5})) // not really needed in this case, but whatever :)

// and the best part, it actually doesn't work outside assignments :DDDD
// all this fails to compile
// cast([])
// cast({a:5})
// cast(NumberArray.create([]))
// cast(A.create({n2: 5}))
// cast({a: 2, b: 5})
// cast(NumberMap({a: 2, b: 3}))

the good thing is that you can actually use the type of the variables themselves, which is helpful for cases like
types.model({ someArray: types.array(types.string) })

with idea 3 you are forced to separate them like

const StringArray = types.array(types.string)
types.model({ someArray: StringArray })....
setSomeArray(...) {
  self.someArray = StringArray.cast([])
}

or do some repetition

setSomeArray(...) {
  self.someArray = types.array(types.string).cast([])
}

with this new idea you don't have to if you don't want to

setSomeArray(...) {
  self.someArray = cast([])
}

to be honest I don't even know how setA5/setArr4 even works, but it does! must be some TS inference magic

not to mention the best part, that **the cast function (without a given type template) doesn't even work outside an assignment 💃 **

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Aug 10, 2018

updated the sample with arrays and maps

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Aug 10, 2018

next questions (if IDEA 4 is liked more, for sure I do)

  • CreationOrInstance or SnapshotOrInstance? this type is just to make it easier to declare parameters
  • cast as function name?

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Aug 10, 2018

to put it into perspective against the other options

IDEA 4

model.a = cast({})
arrayOfA[0] = cast({})
mapOfA.set('a', cast({}))

magic!

@xaviergonz xaviergonz self-assigned this Aug 11, 2018
@xaviergonz xaviergonz added the has PR A Pull Request to fix the issue is available label Aug 11, 2018
@xaviergonz
Copy link
Contributor Author

just made a PR for idea 4

@mweststrate
Copy link
Member

Released, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brainstorming/wild idea Brainstorming / Wild idea has PR A Pull Request to fix the issue is available
Projects
None yet
Development

No branches or pull requests

3 participants