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

add mstRunInAction #1240

Closed
wants to merge 2 commits into from
Closed

add mstRunInAction #1240

wants to merge 2 commits into from

Conversation

xaviergonz
Copy link
Contributor

@xaviergonz xaviergonz commented Mar 31, 2019

  • Added mstRunInAction, similar to mobx's runInAction but for MST node instances.
  • Removed castFlowReturn since when a flow returns a promise what is actually returned from the flow is the resolved value. (this makes mobx and mst flow defs similar)

mstRunInAction(node, name?, thunk)

Usually used to do async/await actions without the need for superfluous actions. Not as good as flow, but has better type inference.

That being said I'm really looking forward Strongly typed iterators and generators in the roadmap for TS 3.5 to finally fix the typings for flow

@coveralls
Copy link

coveralls commented Mar 31, 2019

Coverage Status

Coverage increased (+0.08%) to 92.442% when pulling 23c1918 on mstRunInAction into 462904d on master.

@xaviergonz xaviergonz requested a review from mweststrate March 31, 2019 09:50
@xaviergonz xaviergonz added the require('@mweststrate') @mweststrate input is required! label Apr 1, 2019
@xaviergonz xaviergonz requested a review from k-g-a April 6, 2019 21:33
@mweststrate
Copy link
Member

I'm not a fan of mstRunInAction. I think it encourages anti patterns, which, if needed, can be really easily solved in userland with something along the lines: .actions(self => ({ runAny(thunk) { return thunk(self) } } ). I think mstRunInAction might bless an approach that generally should be avoided. Did you have any direct use case?

@mweststrate
Copy link
Member

The example mentioned in the docs would benefit from either using flow, or splitting in separate actions imho

@xaviergonz
Copy link
Contributor Author

To be honest if flows were to be properly typed in TS (or if async/await were to work ok) I'd not have much of a use case. I usually skip flows in the original mobx and use runInAction after awaits exclusively for this reason (yield always returning any).

Another alternative is using anonymous actions, with private actions that are not exposed, but that's not possible unless there's some way in models to create those (which mstRunInAction would allow).

Maybe a better way to do this is to rename it to mstAction and make an example like this?

actions(self => {
  const privateAction = mstAction(self, (x: number) => { self.x = x })

  return {
    async asyncAction() {
      const x = await something()
      privateAction(x)
    }
  }
})

@jamonholmgren
Copy link
Collaborator

@xaviergonz Any update on this? I'm taking over MST as primary maintainer, just checking to see if you still want to pursue this.

@jamonholmgren jamonholmgren added the question Question or help request label Nov 17, 2020
@xaviergonz
Copy link
Contributor Author

Sorry, feel free to close it

@mattruby mattruby closed this Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Question or help request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants