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

Improving Typescript usability? #1201

Open
zxti opened this issue Mar 6, 2019 · 14 comments
Open

Improving Typescript usability? #1201

zxti opened this issue Mar 6, 2019 · 14 comments
Labels
can't fix Cannot be fixed never-stale Should never be marked as stale by the bot Typescript Issue related to Typescript typings

Comments

@zxti
Copy link

zxti commented Mar 6, 2019

Is your feature request related to a problem? Please describe.
MST sounds awesome. However, I've tried a couple times to start using MST in my TS codebases, but invariably abandon due to Typescript issues (at least as far as I know, and I don't know much).

A few examples:

  • Circular references in structure need casts sprinkled at all use sites.
  • Can't reference self.otherAction() from an action (again, without casts sprinkled).
  • Can't accept nor return recursive/self types in actions/views.
  • Slow type inference.
  • Large, cryptic type errors.
  • Spotty type-checking (sorry I know this is vague, but this is what I seem to have experienced).
  • Flow generators are non-standard and thus don't benefit from things like tslint's no-floating-promise.
  • Some other things I'm forgetting (might be useful to have a roundup that's more complete than what's in the README currently).

There are also opportunities for improved typing, such as models defaulting to read-only types outside of actions.

Describe the solution you'd like
I understand that some of these are chalked up to Typescript's limitations, but IMO improved TS usability is a worthy target given TS's momentum. At least for this user, TS usability is the main immediate barrier to adoption.

A different approach from embedded type system is code generation. Various GraphQL tools use this approach, as a random example.

I know this doesn't solve everything, e.g. flow generators, but I imagine it would be a big step up.

Downsides include more frontend infrastructure needed to execute code generation.

Describe alternatives you've considered

  • Using MobX directly, but then you can't have the nice things MST brings.
  • Casting everywhere.
  • Perhaps a layer of indirection in MST that applies some forceful casting at the right places, so that the types exposed to the user are "clean." I haven't thought this through enough, but I suspect this may be intractable.

Are you willing to (attempt) a PR?
You wouldn't want a change like this in my hands....

@ksjogo
Copy link

ksjogo commented Mar 6, 2019

Do you know https://github.com/charto/classy-mst ?
I am using this inside my TS projects and it improves TS experience a lot.

@zxti
Copy link
Author

zxti commented Mar 6, 2019

@ksjogo Thanks, yeah that was actually one of the first ways I tried using MST - IIRC it seemed to only help with the self.otherAction problem. (Also I swear by https://github.com/alangpierce/sucrase, and classy is cumbersome to try to use without decorators.)

@xaviergonz
Copy link
Contributor

xaviergonz commented Mar 7, 2019

Circular references in structure need casts sprinkled at all use sites.

true :(

There are some workarounds though:

const Node = types.model({
    x: 5, // as an example
    me: types.maybe(types.late((): IAnyModelType => Node))
})

node.((me) as Instance<typeof Node>).x // x here will be number

or with an extra view and an interface

const Node = types.model({
    x: 5, // as an example
    me: types.maybe(types.late((): IAnyModelType => Node))
}).views(self => ({
  get _me(): INode { return self._me }
}))

interface INode extends Instance<typeof Node> {}

node._me!.x // x here will be number
node._me!._me!.x // same

None of them are pretty though and none of them help with the typing of the snapshots though

Can't reference self.otherAction() from an action (again, without casts sprinkled).

just use this.otherAction() instead if the otherAction is declared on the same block, or if it is declared on a previous block use self.otherAction (same for views)

Can't accept nor return recursive/self types in actions/views.

You should be able if you use the interface trick (at least for returning, not sure above accepting) (see above)

Slow type inference.

I thought that was fixed from MST 3.8 (at least in vscode), which IDE?

Large, cryptic type errors.

true too :(

Spotty type-checking (sorry I know this is vague, but this is what I seem to have experienced).
Flow generators are non-standard and thus don't benefit from things like tslint's no-floating-promise.
Some other things I'm forgetting (might be useful to have a roundup that's more complete than what's in the README currently).

Yeah, it is a shame that typescript doesn't support typing return arguments from generator functions. Until they add support for that there's no way to fix that. It is always possible to use an async function and use runInAction after each await to cover any self mutations though (or use another sync action) instead of flows

@xaviergonz
Copy link
Contributor

xaviergonz commented Mar 7, 2019

That being said (about recursive props) I was making some tests and I found a way to "kind of" make it work

    type Omit<T, K> = Pick<T, Exclude<keyof T, K>>

    type RecurseMe<T, P extends keyof T> = Omit<T, P> & { [k in P]?: RecurseMe<T, P> }

    type ModelWithRecursiveProps<IT extends IAnyType, P extends keyof SnapshotIn<IT>> = IType<
        RecurseMe<SnapshotIn<IT>, P>,
        RecurseMe<SnapshotOut<IT>, P>,
        RecurseMe<Instance<IT>, P>
    >

    const M = types
        .model({
            x: types.number,
            m: types.maybe(types.late((): IAnyType => M)),
        })
        .actions(self => ({
            setX(v: typeof self.x) {
                self.x = v
            }
        }))

    const RecursiveM: ModelWithRecursiveProps<typeof M, "m"> = M

    const m = RecursiveM.create({ x: 5, m: { x: 6, m: { x: 7 } } }) // snapshots are properly type checked
    const n = m.m!.m!.x // number
    const n1 = m.m!.setX(5) // action is there
    const n2 = m.x // number too

@mweststrate do you think such type helper (ModelWithRecursiveProps) is worth adding to MST?

@zxti
Copy link
Author

zxti commented Mar 7, 2019

@xaviergonz Thanks for sharing that. However I just want to point out that none of my real-world use cases are such simple cases of self-recursion, but usually involve multiple co-recursive structures - it's just that the self-recursive case is the simplest to illustrate as an example.

I do use VSCode. You're probably right in it being fixed by now, this was something I remembered from last time I tried.

Ah, I missed the this trick (and it seems to work with auto-binding) - only slightly annoying to have to switch between self and this, but I can live with that.

Thanks for acknowledging the other issues!

@xaviergonz
Copy link
Contributor

xaviergonz commented Mar 8, 2019

Thanks to you for pointing them out, however they are known and I don't think they can be fixed until TS changes something... (or somebody comes up with some idea)

@xaviergonz xaviergonz added Typescript Issue related to Typescript typings can't fix Cannot be fixed labels Mar 8, 2019
@zxti
Copy link
Author

zxti commented Mar 8, 2019

Well, what do you think about the codegen approach?

@stale
Copy link

stale bot commented Mar 18, 2019

This issue has been automatically marked as stale because it has not had recent activity in the last 10 days. It will be closed in 4 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issue due to inactivity label Mar 18, 2019
@xaviergonz xaviergonz added the never-stale Should never be marked as stale by the bot label Mar 19, 2019
@stale
Copy link

stale bot commented Mar 19, 2019

This issue has been automatically unmarked as stale. Please disregard previous warnings.

@stale stale bot removed the stale Stale issue due to inactivity label Mar 19, 2019
@kresli
Copy link

kresli commented Mar 28, 2019

what I'm doing for circular is

const Node = types.model({
    x: 5, // as an example
    me: types.maybe(types.late((): IAnyModelType => Node))
}).views( self => ({
  get node() {
    return self.me as Instance<typeof Node>
  }
})
Node.create({}).node // typed me :)

@evelant
Copy link

evelant commented Sep 13, 2019

What I'm doing for async is this:

.actions(self => ({
    runInAction(actionName: string, fn: () => any) {
            log.info(`runInAction from ${actionName}`)
            return Promise.resolve(fn())
    },
)})

Then I can make any action async, all I have to do is wrap any modifications to the store using

await self.runInAction("loggingName", () => { self.foo = "bar" })

This is far easier for me than trying to deal with flow and generators which I could never make TS behave well with.

@mweststrate
Copy link
Member

mweststrate commented Sep 13, 2019 via email

@zuhorer
Copy link

zuhorer commented Sep 19, 2023

issue moved to : coolsoftwaretyler/mst-middlewares#10

@coolsoftwaretyler
Copy link
Collaborator

Hey folks - this is not actually being moved. It got caught up in the issue migration because it matches middleware when searching open issue. Leaving this open for now. We are definitely planning on improving TS here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can't fix Cannot be fixed never-stale Should never be marked as stale by the bot Typescript Issue related to Typescript typings
Projects
None yet
Development

No branches or pull requests

8 participants