fixes for TS types and added types to unit tests #937

xaviergonz · 2018-07-20T00:24:26Z

A (kinda big) pull request to fix lots of TS typing issues
Oh, and also added types to all tests

As a plus I publicly exported TestFlags so mst-middlewares won't complain that it can't find them.

A couple of important things:

I made frozen T part of C,S,T deeply immutable (deep immutable array for arrays, deep immutable map for maps, deep immutable object for objects and nothing for already immutable primitives, all in a recursive manner)
Made arrays and maps optional (as per docs)
Made frozen(default) optional, while frozen() is not
Frozen doesn't automatically add "| undefined" as valid creation type, the user has to actually provide it.
Lots of other fixes actually.

Fixes: #923 #930 #932 - maybe #922

Btw, shouldn't custom type actually use <C, S, T> rather than <S|T,S,T> for more flexibility?

xaviergonz · 2018-07-20T16:12:08Z

There's one unit test failing now that wasn't failing before adding TypeFlags to exports):

it should emit snapshots for children
but weirdly enough it also fails now when I run the tests over the master branch...

 it should emit snapshots for children

    expect(received).toEqual(expected)

    Expected value to equal:
      {"files": [{"name": "01-arrival"}, {"name": "Photo2"}], "name": "Vacation photos"}
    Received:
      {"name": "01-arrival"}

    Difference:

    - Expected
    + Received

      Object {
    -   "files": Array [
    -     Object {
        "name": "01-arrival",
    -     },
    -     Object {
    -       "name": "Photo2",
    -     },
    -   ],
    -   "name": "Vacation photos",
      }

      139 |     onSnapshot(folder.files[0], snapshot => snapshots.push(snapshot))
      140 |     folder.files[0].rename("01-arrival")
    > 141 |     expect(snapshots[1]).toEqual({
          |                          ^
      142 |         name: "Vacation photos",
      143 |         files: [{ name: "01-arrival" }, { name: "Photo2" }]
      144 |     })

xaviergonz · 2018-07-20T16:33:19Z

Apparently the error was an accidental change to mobx-state-tree package-json that changed mobx version to 4.3.1 from 5.0.3.
I restored it and now everything seems to be fine.

xaviergonz · 2018-07-20T17:11:28Z

besides making TypeFlags public and exported also added TypeFlags imports to undo-manager and the other mst-middleware to fix #930

xaviergonz · 2018-07-20T17:14:06Z

Also fixes #932

xaviergonz · 2018-07-20T18:01:55Z

just added a fix (with unit test) for optional enums inside a model crashing
in other words, the following will crash:

test("should support optional enums inside a model", () => {
    const TrafficLight = types.model({
        color: types.optional(types.enumeration(["Red", "Orange", "Green"]), "Orange")
    })
    const l = TrafficLight.create({})
    expect(l.color).toBe("Orange")
})

because types.optional would inherit as flag TypeFlags.Union, but not "types" and therefore crash here

function tryGetOptional(type: any): OptionalValue<any, any, any> | undefined {
    if (!type) return undefined
    if (type.flags & TypeFlags.Union) return type.types.find(tryGetOptional) // THIS LINE
    if (type.flags & TypeFlags.Late) return tryGetOptional(type.subType)
    if (type.flags & TypeFlags.Optional) return type
    return undefined
}

so I fixed it like this

function tryGetOptional(type: any): OptionalValue<any, any, any> | undefined {
    if (!type) return undefined
    // we need to check for type.types since an optional union doesn't have direct subtypes
    if (type.flags & TypeFlags.Union && type.types) return type.types.find(tryGetOptional)
    if (type.flags & TypeFlags.Late && type.subType) return tryGetOptional(type.subType)
    if (type.flags & TypeFlags.Optional) return type
    return undefined
}

another option would be to make optional just return the optional flag, but I'm not sure if that's better

mweststrate · 2018-07-20T18:35:50Z

Thanks for the investigation! I think the solution is correct, will double check next week! Op vr 20 jul. 2018 om 20:01 schreef Javier Gonzalez < [email protected]>:

…

just added a fix (with unit test) for optional enums inside a model crashing in other words, the following will crash: test("should support optional enums inside a model", () => { const TrafficLight = types.model({ color: types.optional(types.enumeration(["Red", "Orange", "Green"]), "Orange") }) const l = TrafficLight.create({}) expect(l.color).toBe("Orange") }) because types.optional would inherit as flag TypeFlags.Union, but not "types" and therefore crash here function tryGetOptional(type: any): OptionalValue<any, any, any> | undefined { if (!type) return undefined if (type.flags & TypeFlags.Union) return type.types.find(tryGetOptional) // THIS LINE if (type.flags & TypeFlags.Late) return tryGetOptional(type.subType) if (type.flags & TypeFlags.Optional) return type return undefined } so I fixed it like this function tryGetOptional(type: any): OptionalValue<any, any, any> | undefined { if (!type) return undefined // we need to check for type.types since an optional union doesn't have direct subtypes if (type.flags & TypeFlags.Union && type.types) return type.types.find(tryGetOptional) if (type.flags & TypeFlags.Late && type.subType) return tryGetOptional(type.subType) if (type.flags & TypeFlags.Optional) return type return undefined } another option would be to make optional just return the optional flag, but I'm not sure if that's better — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#937 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhG2ySiJCLS3LoSoN1meNrL0Wjogaks5uIhsVgaJpZM4VXR1D> .

xaviergonz · 2018-07-20T19:08:23Z

Maybe fixes #922

xaviergonz · 2018-07-22T19:04:31Z

Ok, basically I took the opportunity to make a revamp of the typescript typing to further improve it.
Notable changes is that ISnapshottable is no longer there and its role has been taken by IStateTreeNode<C=any,S=any>

With these changes now getSnapshot is properly typed without the need to use its generic argument, references and identifiers work properly and lots of other little fixes.

While there are some small breaking changes about typings, the core API is not changed at all.

xaviergonz · 2018-07-23T10:00:54Z

I made this available in npm as @xaviergonz/mobx-state-tree @xaviergonz/mst-middlewares for anybody that might want to give it a try btw

xaviergonz · 2018-07-23T13:51:24Z

I had to remove DeepImmutable. While cool (made snapshots from getSnapshot unmodifiable, etc) I have a large project that uses MST and it was getting out of ram errors while compiling because of it.

That being said, I'm quite happy with the current state of the typings 👍 so I don't think I'll make further changes.

mweststrate · 2018-07-25T14:28:40Z

@xaviergonz Thanks for the awesome work! Will play a bit with and release

xaviergonz · 2018-07-25T16:53:55Z

@mweststrate my pleasure :)

mweststrate · 2018-07-26T07:56:52Z

Took a close look and these are great improvements! Really awesome work. I've just send you a maintainers invite, that will simplify the workflow for any future contributions a lot, as you can directly push, branch etc on this repo :-)!

dpnolte · 2018-07-26T10:52:59Z

@xaviergonz thanks this fixes my issue 👍
I hope this get published soon on npm.

marvwhere · 2018-07-26T14:04:31Z

cool thx - tried it out (v3.0.2)

looks like it's causing some problems with fork-ts-checker-webpack-plugin - "Starting type checking and linting service..." will never finish and creating a node instance with >100% cpu

went back to 2.2.0 (working)
updated to 3.0.0 (working)
updated to 3.0.1 (not working)

Typescript 2.9
tslint 5.11.0

mweststrate · 2018-07-26T14:16:19Z

and if you disable tslint? Op do 26 jul. 2018 om 16:04 schreef Marvin <[email protected]>:

…

cool thx - tried it out (v3.0.2) looks like it's causing some problems with fork-ts-checker-webpack-plugin - "Starting type checking and linting service..." will never finish and creating a node instance with >100% cpu went back to 2.2.0 (working) updated to 3.0.0 (working) updated to 3.0.1 (not working) Typescript 2.9 tslint 5.11.0 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#937 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhPzuhFVPpsRD7snAf5q4Y4rEJgT3ks5uKcxxgaJpZM4VXR1D> .

xaviergonz · 2018-07-26T15:48:30Z

I have seen that happen from time to time when there are lots of compilation errors. Basically the default node memory of node processes is around 512mb and complex type checking takes lots of ram and then crash.

Things I'd try include:

run tsc by itself on the command line: npx tsc -p .
give it more ram if it dies because of out of ram: NODE_OPTIONS=--max-old-space-size=4096 && tsc -p .
make use of the speed up trick (interface instead of typeof)

type CreationType = typeof SomeModel.CreationType
type SnapshotType = typeof SomeModel.SnapshotType
type Type = typeof SomeModel.Type
export interface ISomeModelCreation extends CreationType {}
export interface ISomeModelSnapshot extends SnapshotType {}
export interface ISomeModel extends Type {}

then using ISomeModelCreation, ISomeModelSnapshot, ISomeModel

or I guess TS could get their act together and optimize that :)

Bnaya · 2018-07-26T18:38:56Z

import { types, getSnapshot } from "mobx-state-tree";

const Square = types
    .model(
        "Square",
        {
            width: types.number
        }
    )

const a = getSnapshot(Square)

I'm getting this error for the getSnapshot call
Type 'IModelType<ModelPropertiesDeclarationToProperties<{ width: ISimpleType<number>; }>, {}, ModelCrea...' has no properties in common with type 'IStateTreeNode<any, {}>'.

I'm probably missing something :)

mweststrate · 2018-07-26T18:43:11Z

Yes, you are snapshoitting a type, not an instance Op do 26 jul. 2018 20:38 schreef Bnaya Peretz <[email protected]>:

…

import { types, getSnapshot } from "mobx-state-tree"; const Square = types .model( "Square", { width: types.number } ) const a = getSnapshot(Square) I'm getting this error for the getSnapshot call Type 'IModelType<ModelPropertiesDeclarationToProperties<{ width: ISimpleType<number>; }>, {}, ModelCrea...' has no properties in common with type 'IStateTreeNode<any, {}>'. I'm probably missing something :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#937 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhIT0NtIxj1xFFgDcdTxrINOzVkhdks5uKgzBgaJpZM4VXR1D> .

Bnaya · 2018-07-26T18:48:02Z

Of Course😂 thanks

…

On Thu, Jul 26, 2018, 21:43 Michel Weststrate ***@***.***> wrote: Yes, you are snapshoitting a type, not an instance Op do 26 jul. 2018 20:38 schreef Bnaya Peretz ***@***.***>: > import { types, getSnapshot } from "mobx-state-tree"; > const Square = types > .model( > "Square", > { > width: types.number > } > ) > const a = getSnapshot(Square) > > I'm getting this error for the getSnapshot call > Type 'IModelType<ModelPropertiesDeclarationToProperties<{ width: > ISimpleType<number>; }>, {}, ModelCrea...' has no properties in common with > type 'IStateTreeNode<any, {}>'. > > I'm probably missing something :) > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > < #937 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/ABvGhIT0NtIxj1xFFgDcdTxrINOzVkhdks5uKgzBgaJpZM4VXR1D > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#937 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPpHuNjGJhGVxQ-6E5G6EqUhyb_Fadxks5uKg3BgaJpZM4VXR1D> .

marvwhere · 2018-07-27T08:29:56Z

@xaviergonz I tried around, but still not helping. The strange thing is that it is working with 3.0.0. I used that version to remove all lint errors which popped up through the breaking changes. then went back to 3.0.2 - still not working (i mean it is compiling, but the linter is not working)

I try to play around with it a little bit and try to find out which commit breaks it.

xaviergonz · 2018-07-27T09:26:25Z

so the compiler works but the linter doesn't? what if you just run tslint stand alone?

mweststrate · 2018-07-27T09:32:00Z

Sounds more like an issue to report at the linter library? Op vr 27 jul. 2018 11:26 schreef Javier Gonzalez <[email protected]>:

…

so the compiler works but the linter doesn't? what if you just run tslint stand alone? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#937 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhJXOdJN90I1WB0qPKfD_8awc_5v3ks5uKtzdgaJpZM4VXR1D> .

marvwhere · 2018-07-27T09:47:43Z

kind of yes =/

NODE_OPTIONS=--max-old-space-size=4096 && npx tsc -p . command

working fine in 3.0.0 and does nothing in 3.0.1

xaviergonz added 2 commits July 20, 2018 02:22

fixes for TS types and added types to unit tests

239d702

exported TypeFlags

eaed13b

xaviergonz mentioned this pull request Jul 20, 2018

add Typeflags to exports #931

Closed

xaviergonz added 2 commits July 20, 2018 18:27

restored accidental change to package.json

31bfb95

restored formatting to package.json

bf5c0c0

fix problem with mst middlewares and TypeFlags

30d8ce6

mobxjs deleted a comment from xaviergonz Jul 20, 2018

fix for optional enum inside model being broken

935e600

xaviergonz mentioned this pull request Jul 20, 2018

TS Error with types.optional in 3.0.0 #923

Closed

6 tasks

xaviergonz and others added 5 commits July 21, 2018 11:38

fix for setLivelynessChecking tip message mispelling

798cb2e

forgot to cast date to IComplexType

ea85231

revamp of the TS type system

be4895d

CI compilation fixes

d209a1f

updated ts to 2.9.2

10c0131

fix for resolveIdentifier/getParentOfType

97c8678

xaviergonz added 5 commits July 23, 2018 13:12

fix for applySnapshot

11ffb2c

tiny type fix for unit test

a8525db

more fixes for getSnapshot and applySnapshot

fd66d19

tiny fixes

cc6285a

removed DeepImmutable as it makes large projects compilation slow

e018630

mweststrate merged commit 708eb5a into mobxjs:master Jul 25, 2018

This was referenced Jul 26, 2018

Error with optional and compose models. Cannot read property 'find' of undefined #922

Closed

(Typescript) frozen fields without a default always include undefined as a possible type #932

Closed

xaviergonz deleted the ts-typing-fixes branch August 25, 2018 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes for TS types and added types to unit tests #937

fixes for TS types and added types to unit tests #937

xaviergonz commented Jul 20, 2018 •

edited

Loading

xaviergonz commented Jul 20, 2018 •

edited

Loading

xaviergonz commented Jul 20, 2018

xaviergonz commented Jul 20, 2018

xaviergonz commented Jul 20, 2018

xaviergonz commented Jul 20, 2018

mweststrate commented Jul 20, 2018 via email

xaviergonz commented Jul 20, 2018

xaviergonz commented Jul 22, 2018 •

edited

Loading

xaviergonz commented Jul 23, 2018

xaviergonz commented Jul 23, 2018

mweststrate commented Jul 25, 2018

xaviergonz commented Jul 25, 2018

mweststrate commented Jul 26, 2018

dpnolte commented Jul 26, 2018

marvwhere commented Jul 26, 2018

mweststrate commented Jul 26, 2018 via email

xaviergonz commented Jul 26, 2018

Bnaya commented Jul 26, 2018

mweststrate commented Jul 26, 2018 via email

Bnaya commented Jul 26, 2018 via email

marvwhere commented Jul 27, 2018

xaviergonz commented Jul 27, 2018

mweststrate commented Jul 27, 2018 via email

marvwhere commented Jul 27, 2018

fixes for TS types and added types to unit tests #937

fixes for TS types and added types to unit tests #937

Conversation

xaviergonz commented Jul 20, 2018 • edited Loading

xaviergonz commented Jul 20, 2018 • edited Loading

xaviergonz commented Jul 20, 2018

xaviergonz commented Jul 20, 2018

xaviergonz commented Jul 20, 2018

xaviergonz commented Jul 20, 2018

mweststrate commented Jul 20, 2018 via email

xaviergonz commented Jul 20, 2018

xaviergonz commented Jul 22, 2018 • edited Loading

xaviergonz commented Jul 23, 2018

xaviergonz commented Jul 23, 2018

mweststrate commented Jul 25, 2018

xaviergonz commented Jul 25, 2018

mweststrate commented Jul 26, 2018

dpnolte commented Jul 26, 2018

marvwhere commented Jul 26, 2018

mweststrate commented Jul 26, 2018 via email

xaviergonz commented Jul 26, 2018

Bnaya commented Jul 26, 2018

mweststrate commented Jul 26, 2018 via email

Bnaya commented Jul 26, 2018 via email

marvwhere commented Jul 27, 2018

xaviergonz commented Jul 27, 2018

mweststrate commented Jul 27, 2018 via email

marvwhere commented Jul 27, 2018

xaviergonz commented Jul 20, 2018 •

edited

Loading

xaviergonz commented Jul 20, 2018 •

edited

Loading

xaviergonz commented Jul 22, 2018 •

edited

Loading