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

async/await simplification with flow and babel in strictMode #1410

Closed
PDarkTemplar opened this issue Mar 15, 2018 · 23 comments
Closed

async/await simplification with flow and babel in strictMode #1410

PDarkTemplar opened this issue Mar 15, 2018 · 23 comments

Comments

@PDarkTemplar
Copy link

PDarkTemplar commented Mar 15, 2018

For example you have store code like this:

class Test {
    @action
    async login() {
        globalStore.user.loading = true;
        const result = await api.get('http://mycoolapi.org/get/1');
        runInAction(() => {
            globalStore.user.loading = false;
            globalStore.user.data = result;
        });
    }
}

if you use @babel/plugin-transform-async-to-generator
with this configuration in your babelrc:

...
            "plugins": [
                ["@babel/plugin-transform-async-to-generator", {
                    "module": "mobx",
                    "method": "flow"
                  }],
            ]

you can simplify your code to this:

class Test {
    async login() {
        globalStore.user.loading = true;
        const result = await api.get('http://mycoolapi.org/get/1');
        globalStore.user.loading = false;
        globalStore.user.data = result;
    }
}

And all will be working like a charm :)

@mweststrate
Copy link
Member

That's genious!

@caesarsol
Copy link

Wow, I didn't know of this babel feature!
Isn't this goong to mangle with any other async/await use though? :)

@PDarkTemplar
Copy link
Author

Yes, it would. This is the downside of this method. Do you see any problem with this approach?

@caesarsol
Copy link

So it would wrap in mobx actions every async function, even the ones unrelated to mobx observable's mutations.

Shouldn't introduce unexpected things, but I think you may incur in performance penalties!

If there could be a way to "tag" that async function as action and make the transform act only on it, it would be very cool.

What do you think? Are you currently using this trick?

@PDarkTemplar
Copy link
Author

PDarkTemplar commented Mar 21, 2018

Yes, i am currently using this approach, but because all my async code essentially modifing mobx observables i would use runInAction anyway. Regarding perfomance, you mean penalties from action wrap or from transpiling to generators?

About tagging, i think it should not be very hard to create custom plugin, which will be using plugin-transform-async-to-generator under the hood but with test that async wrapped in action.

@caesarsol
Copy link

Yes, I mean penalties from action wrap!

I'm curious on what @mattiamanzati thinks about this :)

@PDarkTemplar
Copy link
Author

PDarkTemplar commented Mar 22, 2018

Here is my small proof of concept for babel 7.0.0-beta.42:
raw
compiled

this babel transform apply mobx flow, only to functions with // action or /* action */ comments.

// action
async login() {
    await test();
}

/* action */
async login1() {
    await test();
}

@Strate
Copy link
Contributor

Strate commented May 7, 2018

Wow, didn't check this issue exists. Just released my plugin, which can handle typescript's emitted decorator too: https://github.com/Strate/babel-plugin-mobx-async-action

@Obi-Dann
Copy link
Contributor

FYI, We did a TS transformer plugin for transforming async functions into generators wrapped into mobx flow https://github.com/AurorNZ/ts-transform-async-to-mobx-flow

@harvey-woo
Copy link

Here is my small proof of concept for babel 7.0.0-beta.42:
raw
compiled

this babel transform apply mobx flow, only to functions with // action or /* action */ comments.

// action
async login() {
    await test();
}

/* action */
async login1() {
    await test();
}

Can this solution also be implemented by checking @action, it seems more reasonable

@jjinux
Copy link
Contributor

jjinux commented Dec 10, 2018

@mweststrate, etc. can you provide some guidance on whether or not you think this Babel approach will pan out? @joecan and I have a large, existing codebase we're porting. We're currently on MobX 3, but we can upgrade as necessary. We're not fans of the flow approach because it's a little too confusing to explain when it's necessary and because we think our guys understand async/await syntax better than generator syntax. The runInAction approach is easy to understand, but it's a bit verbose (it'll add 2 lines per instance X 800 instances). We're on Babel 7, so if the Babel approach takes off, that is probably best, but we don't know how long to wait for it. We already switched our spec files to use async/await. It's really the MobX files that are holding us back.

@harvey-woo
Copy link

Hi, @jjinux. @PDarkTemplar has provided a workable solution

@jjinux
Copy link
Contributor

jjinux commented Dec 11, 2018

Thanks, @harvey-woo :)

@sealzrt
Copy link

sealzrt commented Jan 18, 2019

thanks

@brandonk3nt
Copy link

brandonk3nt commented Jan 28, 2019

wouldn't this be a good candidate for the new flow api?
https://mobx.js.org/refguide/api.html#flow

It implements generators without having to configure babel.

@PDarkTemplar
Copy link
Author

@codemerc It is using new flow api under the hood. This plugin allows default async await, without generators, very usefull with typings.

@arvigeus
Copy link

arvigeus commented Mar 3, 2019

@PDarkTemplar I was unable to make your code work, but the proposed solution in the first post worked well enough

@DarkCow
Copy link

DarkCow commented Apr 26, 2019

I forked AurorNZ/ts-transform-async-to-mobx-flow, and made https://github.com/DarkCow/ts-transform-async-to-mobx-flow/tree/feature/action-decorators

It works on @action decorator instead, Thanks everyone here! This is really cleaning up my codebase.

Input

class Test {
  value: string = '';

  @action
  async func(input: string) {
    this.value = await Promise.resolve(input);
  }
}

Output

class Test {
  func(input) {
    return flow_1(function* func() {
      this.value = yield Promise.resolve(input);
    }).call(this);
  }
}

edit: should note I only just forked it, haven't published it, and since it is so divergent from AurorNZ code, not sure if an MR is warranted...

@jjinux
Copy link
Contributor

jjinux commented Apr 26, 2019

I'm super excited to try this out! 🥇

@Obi-Dann
Copy link
Contributor

@DarkCow the reason @transformToMobxFlow was chosen as the name to make it quite unique to avoid name clashes (action is quite a common function name), to make it very clear that it is not the standard behaviour of action and to avoid handling named imports, eg.

import {action as action2} from 'mobx';

action2(async function(){}) // that wouldn't be transformed because the code only checks for keyword `action`

@ItamarShDev
Copy link
Member

This thread is awesome.
I agree with @DanNSam. @action is commonly used and wrapping it can create bugs that would be hard to realise.

Maybe rename to @asyncAction or @DanNSam idea

@Tsury
Copy link

Tsury commented May 15, 2019

Btw, there seem to be another issue with name conflicts:

import { someFunc } from './someFile';

class SomeClass {
  @transformToMobxFlow
  someFunc = async () => {
    await someFunc();
  }
}

This code will run fine without @transformToMobxFlow. When adding it, after it is transformed, the inner call await someFunc() will reference the transformed function, and not the imported one. Is there a way to resolve this? I know it's bad practice to have conflicting names, but these are different scopes and this can happen.

@stale
Copy link

stale bot commented Jun 19, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests