MatAtBread / fast-async

605 stars 21 forks source link

Babel: Plugin no longer emits Function.prototype.$asyncbind since 6.0.34 #24

Closed ephemer closed 7 years ago

ephemer commented 7 years ago

As in the title. The plugin has been unusable since 6.0.34 for us because - although it seems to transform the async functions correctly - it doesn't provide the core runtime functionality ($asyncbind).

We only output $asyncbind to one file as per the docs:

{
    "presets": ["es2015"],
    "plugins": [
        ["fast-async", {
            "runtimePattern": "src/index.js"
        }]
    ]
}

In 6.0.34 this works. In 6.1+ the transform appears to work but src/index.js doesn't receive $asyncbind. This may be because src/index itself doesn't contain any async functions itself?

nightwolfz commented 7 years ago

Remove runtimePattern and it should work ?

matAtWork commented 7 years ago

Quite possibly. This was implemented in https://github.com/MatAtBread/fast-async/commit/2d60c327e7a72bff8b726ba7f53d1d35821fb0c3 as part of an attempt to rationalise the whole runtime issue.

The preferred mechanism for some users is to simply require('nodent-runtime') in your app - this is now a separate module (quite small, without the compiler and other stuff).

Alternatively, just make sure your runtime-pattern actually refers to a file that has an await or async function in it, even if they are never executed.

FYI, I really try not to break people's existing code, and apologise for not seeing this one coming(!)

ephemer commented 7 years ago

@nightwolfz we did that at the start - it doubles our build time and significantly increases bundle size.

@matAtWork so you think it's because src/index.js contains no async code? I'd be happy to do this with the runtime import, it's just that I just don't understand the docs, in particular I don't understand the option useRuntimeModule. What concretely do I have to do?

npm install nodent-runtime --save in src/index.js import 'nodent-runtime';

? Do I then remove runtimePattern from .babelrc and add useRuntimeModule: true?

To me this initially looked like an option that would have a negative impact on performance, but from this short discussion I'm slowly gathering that maybe all it does is add $asyncbind to Function.prototype?

I thought it'd be safe to assume that pretty much every project that uses fast-async has one defined entry point and that it'd just make sense to dump the Function.prototype stuff in there (which is what I tried to do with runtimePattern). In which case I'd just call the option entryPoint. But maybe I'll go down the runtimeModule path if that's the new recommendation?

matAtWork commented 7 years ago

Yes, a test was added to only produce the runtime if the file actually contained references to it. My bad.

The 'useRuntimeModule' option makes it generate require('nodent-runtime') instead of the actual code - you still need a pattern (or directive) to get anything generated.

I'd suggest changing to runtimePattern:'none' and manually require('nodent-runtime') in your app - it's just cleaner and easier to manage as a simple dependency.

At yes, the inline code (or generated require, or manually require'd) just initialise Function.prototype.$asyncbind and Function.prototype.$asyncspawn.

Finally, in nodent v3, there's the code generation option 'noRuntime' which works with Promises and generates vanilla ES5 with no runtime requirement at the expense of code size and speed.

ephemer commented 7 years ago

Ok great. I did just that and it works. I think it'd be nice to update the docs to give One Clear Path(TM) of doing this. Thanks for your quick feedback on this

matAtWork commented 7 years ago

@ephemer Yes please :) Feel free to tweak the README and add it to your PR.

ephemer commented 7 years ago

@matAtWork I had a quick look at this but it seems like it'd be a pretty significant overhaul of the docs. Especially with the implication of using an external dependency. It also seems weird to recommend runtimePattern: 'none' as the best practice solution. Seems to me like this would be better to implement in code as a default. Again, this would seem to imply a major version update.

My preferred path would still be to name an entryPoint in .babelrc and simply inline the Function.prototype stuff there. That way there'd be no external dependencies, the file size and build time would be (almost) as small as possible, and the whole thing would remain self-contained.

But I feel like that's beyond the scope of changes you'd want to accept from someone who just contacted you out of the blue for the first time an hour ago :)

I think for now the first pull request is kind of urgent and should go in ASAP. The docs can wait a little

matAtWork commented 7 years ago

I've pulled, fixed a different bug and added a test to check babel-cli actually runs. Hopefully this will get you going again.

matAtWork commented 7 years ago

Published as fast-async@6.1.2