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

Allow action to handle async functions #2153

Closed

Conversation

reaktivo
Copy link

This PR wraps each "step" of an Async function in its own action. This removes the need for flow, rewriting async functions as generators and most uses of runInAction when used in the context of an async function.

In the following example, the both data and loading are run in a single action, removing the need for extra runInAction calls.

Of course, this PR is simply a proof-of-concept and there's quite a few other things to consider:

  • Is there a drastic performance impact in having each async resolver wrapped in an action, even when there's possibly no observable being mutated?
  • If this would ever be merged and supported in MobX, would async functions that already in include runInAction calls cause any issues?
  • Last but not least, is this the correct place to have the resolver's being hooked into?

What do you think?

Instead of

const store = observable({ loading: false, data: {} });
const someAsyncWork = action(async () => {
  store.loading = true;
  const data = await fetchSomething();
  runInAction(() => {
    store.data = data;
    store.loading = false;
  });
});

you can simply write:

const store = observable({ loading: false, data: {} });
const someAsyncWork = action(async () => {
  store.loading = true;
  const data = await fetchSomething();
  store.data = data;
  store.loading = false;
});

PR checklist:

  • Added unit tests
  • Updated changelog
  • Updated /docs. For new functionality, at least API.md should be updated
  • Added typescript typings
  • Verified that there is no significant performance drop (npm run perf)

@danielkcz danielkcz requested a review from xaviergonz October 13, 2019 15:00
@danielkcz
Copy link
Contributor

danielkcz commented Oct 13, 2019

Recently @xaviergonz introduced asyncAction (#2118), perhaps that covers your concern? I haven't investigated it yet, so I am not sure.

@xaviergonz
Copy link
Contributor

It would actually be this PR mobxjs/mobx-utils#217

I also tried to make it magically work by tweaking promises (and actually got quite far) in this other PR mobxjs/mobx-utils#217 but in the end native (non transpiled) async/await bit me.
Using native async/await in Chrome would not call the overridden promise constructor (a first approach I had), and in node it wouldn't call overridden "then", "catch", etc. :(

@reaktivo
Copy link
Author

@xaviergonz You're absolutely right. I was misled by the transpiled output and the tests, which worked seemingly well. Seems there's no way to hook into awaits at all. Thanks for having a look. Closing this one.

@reaktivo reaktivo closed this Oct 13, 2019
@reaktivo reaktivo deleted the reaktivo/allow-async-actions branch October 13, 2019 21:17
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

Successfully merging this pull request may close these issues.

3 participants