MatAtBread / fast-async

605 stars 21 forks source link

Merge this project into the official async plugin for Babel? #46

Closed hzoo closed 6 years ago

hzoo commented 6 years ago

Right now there are a few ways to use (non-native) async in Babel:

We also had a module called async-to-module-method but it seemed weird to make it separate so I just merged that as an option in the other async plugin.

I'd like to propose we just merge everything into 1 plugin?

So in v7 it would be called @babel/plugin-async-functions and it would have an option to compile using generators (async-to-gen) and promises (using a port of this plugin + kneden).

I'd like this because regenerator has to have a runtime, it's large, and still under the BSD+Patents by FB (waiting for them to change it). If in the meantime we can get this working I'm cool with just switching it out by default and other people can opt-in to using regenerator if they want but it would be outside of our repo.

Would that be desirable? Basically for this new plugin I would want everything in fast-async that is related to compiling async to promises but nothing else (so don't need the logic for generators, or any non-spec behavior that is in nodent). Let me know if that would be cool, or if we can get someone else in the community to do this if you are busy

So the new version wouldn't need all of https://github.com/MatAtBread/nodent or https://github.com/MatAtBread/nodent-runtime or the options

matAtWork commented 6 years ago

That would be reasonably easy to do - basically invoke nodent with the options preset to those you require (fast-async already only uses Promises, and has the spec option which disables any extensions and uses default semantics).

hzoo commented 6 years ago

Ok great, then we can deprecate https://github.com/babel/kneden and just add tests to make this happen. Do you want to make the PR?

I was thinking we just rename https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-async-to-generator to transform-async-functions and then add a promise option

hzoo commented 6 years ago

And the promise only version has no runtime right? Basically I would want the plugin to be a straight translation from async functions to native promises?

This would definetely lead to a lot more usage so will probably expect some more issue reports to Babel πŸ˜› , I can add you as collab to assign if you want as well!

matAtWork commented 6 years ago

If you specify the option noRuntime. spec option sets the ones you want. See https://github.com/MatAtBread/fast-async/blob/master/plugin.js#L67

A lot of the plugin code will disappear if you don't want to support the various options at all, including the runtime. Transpilation itself is not very quick (unlike runtime), so it's worth leaving in the test for whether async or await are actually in the AST.

The requirement to avoid a runtime is a common one (which is one reason there's an option in nodent), but the code generated can be quite a bit bigger and sometimes a little slower. The nodent runtime is pretty small (150 lines un-minified) and in a separate repo/npm (see https://github.com/MatAtBread/nodent-runtime) and so can easily be required without much hassle by the generated code, or injected by the transpiler. Much as I understand the attraction of a "pure" transpiler, avoiding the runtime has some real penalties.

I might have some time to at least get this up and running. Do you have a repo within the babel project I can contribute to?

hzoo commented 6 years ago

We would move it into the monorepo but I can make a repo in the org if you want. We can support adding it to babel-runtime or polyfill if it's better too (same as regenerator-runtime in that case).

https://github.com/babel/kneden was the old repo that was transferred.

matAtWork commented 6 years ago

Your choice. Just let me know where you want to code to live.

hzoo commented 6 years ago

Eventually I want it to be in https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-async-to-generator as an option so if you want to just make a PR there that would work. Let me know if you have trouble setting up the repo or you can join our slack slack.babeljs.io, or i'l make a new repo

https://github.com/babel/babel/blob/9ae23639ad12898e3bf2dff7b1d1a9fbd2be15fe/packages/babel-plugin-transform-async-to-generator/src/index.js#L8

Could just make an option and do if (promise) and run

hzoo commented 6 years ago

And can we only use just the transform part? We want native promise, no built-in modification (Function.prototype), pure transform

I think we only need the parts in https://github.com/MatAtBread/nodent-compiler/blob/master/lib/arboriculture.js because we won't need to reparse or reprint? Also in order to handle new proposals correctly we would need this modification

tannerlinsley commented 6 years ago

This would be the greatest thing ever.

hzoo commented 6 years ago

Hmm yeah looking into the code more, would that be cool to just port https://github.com/MatAtBread/nodent-compiler/blob/master/lib/arboriculture.js into a standalone babel plugin since we don't need any of the options (specificallly we hardcode to spec: true which is wrapAwait, noRuntime and promises and es6target since we don't need the arrow function transform or the parser/generator parts.

We can start with just the basic thing as you said (just use those options) but not sure that is the most viable for us long term

MatAtBread commented 6 years ago

I might look into refactoring the code so the generator mode is in a separate file (you'll notice it's a small amount of code as async to generator is trivial), however I don't think I'll maintain a fork that diverges significantly from the base compiler as I have a large number of users who don't use Babel or fast-async, and when issues do arise, fixing complex transforms in two projects is an unnecessary burden.

With a bit of work i can probably work it so you require('nodent/arboriculture'), avoiding all the options, CLI, parsing and output code. How does that fit?

matAtWork commented 6 years ago

...or if I get that far create a new module nodent-transformer which you can reference.

hzoo commented 6 years ago

We can definitely maintain the fork transform if it's just the the transform code, but ya the separate file/pkg can probably work fine as a start! I think we'll have the opposite problem eventually if we do try to make this default since we have a lot of users that would want this πŸ˜›, especially if this can become a regenerator replacement

matAtWork commented 6 years ago

Leave it with me - I'll slice up the code so you can require the minimal bits you want. I should get a chance to have a look this week.

MatAtBread commented 6 years ago

@hzoo Just took a look at the code (it's been a while...), and the current implementation needs access to:

Are these components available to plugins? Where would I find out about the API?

hzoo commented 6 years ago

Sorry we don't have the best docs yet

We have a slack channel you can join: slack.babeljs.io or you can ask here

MatAtBread commented 6 years ago

The templating code at https://github.com/babel/babel/commit/f230497d083b97721152763d87aac4bb317f8b97 is almost identical to the technique I use at https://github.com/MatAtBread/nodent-compiler/blob/master/lib/arboriculture.js#L906, although it's not suitable as I have functional substitutions as well "literal" nodes.

I can't work out if tree-walking can be aborted. What I mean is I have quite a few cases where having recursed into a node, I start a different walker from the current location and on return from that abandon the previous one. Often, the current path is passed into second walker (so parent lookups work), but not always. I couldn't see a way of duplicating this technique.

The output routine is used to describe the errors, but I can externalise the routine so fast-async can do one thing and nodent-compiler another, in order maintain backward compatibility.

The issues templating code mean I'll probably leave it as is for now (changing all the logic and references is complicated), invoking @babel/transform for the templating instead of acorn.

The treeWalk seems to be more complicated. Is there an equivalent of the original acorn treewalker left in babel?

MatAtBread commented 6 years ago

...just noticed path.skip() abandons a sub-tree walk, which is good. How do I invoke a new walker from within an existing one? Once I know that, I think we're good to go.

MatAtBread commented 6 years ago

Ok, I now get the codebase (I think) traverse to sub-walk, babylon for parsing, template for....guess what - these are the essential bits I need. Let me see how I get on. I can see value in moving some of the internal functions to the Babel implementations (to reduce the codebase and keep things in common), but for now I'll try and create wrappers/polyfills so we can transform with the internal code+acorn OR Babylon/traverse.

hzoo commented 6 years ago

Yeah let us know if you need some more help, should have apis for a lot of things given the nature of the project

matAtWork commented 6 years ago

I'm sure some questions will come up. I'm kind of busy at work ATM, but I'll try and find some time ASAP

hzoo commented 6 years ago

All good, might have some folks on break for Thanksgiving anyway πŸ˜€!

MatAtBread commented 6 years ago

Thought I'd take a look at this this weekend. Got a simple issue: how do I include babel packages from my local clone?

hzoo commented 6 years ago

Sorry uh, what do you mean specifically? Within babel itself it already links the packages together with make bootstrap and running make watch otherwise you would use something like npm link?

matAtWork commented 6 years ago

...I'm being a dork - I didn't really understand the (simple) relationship between npm scope and links, and assumed it was more complicated than it is. All good now - starting to make progress.

hzoo commented 6 years ago

Cool, feel free to ask questions here or in our slack and I can guide if there's an API/docs thing you need

matAtWork commented 6 years ago

So, it seems to be working for 95% of the cases.

My first target is to abstract all surrounding/supporting dependancies out from arboriculture (which is the original [4 years?] big, complicated, fragile(!), multi-phase async transform where almost all maintenance takes place). This is going pretty well and I have reasonably thin wrappers around the key Babel equivalents of the existing bespoke (or acorn) implementations so that I don't end up with two different arboricultures which will prove hard to maintain.

I'll tidy them up enough to make it a tolerable implementation. How would you like to see this shipped? As a new babel-fast-async that itself depends on nodent-compiler\arboriculture, or integrated into the existing some existing babel package? Are you expecting any new babel-fast-async to be a separate plugin/repo or do you want to include it under babel\packages?

One current issues:

Any ideas if any babylon maintainers are free to have a go? If not, I can (probably) monkey patch it as I did for acorn, but that's not a nice way to go.

ljharb commented 6 years ago

It’s pretty important imo that await outside of an async function, or yield outside of a generator, not even be possible for babel users to get at - is there any way to allow nodent users to do that, but prevent babel or babylon users from doing so?

matAtWork commented 6 years ago

@ljharb - that's already implemented (await outside async is controlled by a flag, and disabled by default for babel). The question is how to get babylon to parse such code into an AST, in the same way that it allows you to parse 'return 123;' (as an atomic string) outside a function, because transform- and compiler-tool writers need such a facility.

matAtWork commented 6 years ago

@ljharb - why confused?

ljharb commented 6 years ago

@MatAtBread because there's no "sad" emoji that it's already implemented :-p anything users can do, they will do, and if users can get await outside an async function, and publish that code anywhere, that will obstruct top-level await actually making it into the language.

matAtWork commented 6 years ago

Ok...nodent users (including me) already use top-level await, however, I'm not advocating anything.

The babel version of the nodent transformer (fast-async) does not default to that or any other extension from the specification (at the cost of performance in some cases). However, babel users are a creative bunch (have you seen some of the proposals (??!!?!?), so whether it's implemented as an internal option in babylon in order to make something which does follow the spec strikes me as moot (e.g. templated AST substitution containing await). If babylon doesn't do it, I have to (again) re-implement in another way.

Does that explain it?

hzoo commented 6 years ago

babel users are a creative bunch

just to clarify we don't allow people to make whatever ideas they want as syntax plugins, they have to be championed by TC39 (ignoring people who fork the parser, etc which they are free to do)

Allow for it in @babel/template is a separate thing from allowing it in the parser itself. Yeah that's fine to ask/request. We have similar issues/request like: https://github.com/babel/babel/issues/6703

matAtWork commented 6 years ago

Well, that's the beauty of open source, you don't have to "allow" anyone anything, and they don't have to ask permission.

Looking at that issue, I can't quite work out what the status is - should I add the requirement there, or create a new ticket?

Do you have any thoughts on how you want this to be (finally) packaged? Be aware that I'm not really a Babel user, so I don't know what fits in with your user base.

matAtWork commented 6 years ago

Just running my tests against old Nodes, and in v0.10, babylon fails:

/Users/matthew.woolf/git/babel/babel/packages/babylon/lib/index.js:118
      beforeExpr,
                ^
SyntaxError: Unexpected token ,
    at Module._compile (module.js:439:25)

I presume make bootstrap did the compilation of babylon (and the other packages), but seems to have left this ES6 construct in place?

var BinopTokenType =
/*#__PURE__*/
function (_TokenType2) {
  _inheritsLoose(BinopTokenType, _TokenType2);

  function BinopTokenType(name, prec) {
    return _TokenType2.call(this, name, {
      beforeExpr,     // <--- Doesn't look like ES5?
      binop: prec
    }) || this;
  }

  return BinopTokenType;
}(TokenType);

To be clear - I've made no changes to any Babel source files, this is straight from the monorepo.

hzoo commented 6 years ago

Oh I think it's since we started compiling babylon against node 4 since Node 0.10/0.12 are dropped in Babel 7.

matAtWork commented 6 years ago

@hzoo - how do you want this to be integrated into babel. As a package in the monorepo? An independent plugin?

hzoo commented 6 years ago

The ideal eventually is a package in the monorepo, I was thinking we would make a package called @babel/plugin-transform-async-functions with some options, one being promises and another generators (the current plugin which is named async-to- generators), so if you want to start with a separate repo that's fine but yeah would be good as a PR to the existing plugin

matAtWork commented 6 years ago

@hzoo - I'm 99% done and have some test harness code that works fine with 7.0.0-beta34. My final 'decision' is whether to fork arboriculture.js, leaving one in nodent-compiler and a new one in babel-transform-async-to-promises, or move it into a new module (nodent-transform) so it can be required from both modules.

The pros and cons for me are simple, but might conflict with your medium-term goals.

Do you have a preference?

ljharb commented 6 years ago

It seems like a shared module is much better; babel could always fork that later if it becomes necessary.

hzoo commented 6 years ago

Ok let's go with the shared/separate module for now, I agree with ljharb. Not really sure what to think until it works and people are using it etc I think?

matAtWork commented 6 years ago

That's fine. The real question is whether you think anyone in Babel will want to put in the time and effort to migrate to the core Babel functions and migrate any fixes. If yes, a fork is what you want to enable those changes (which I won't make in the original as it create an unnecessary dependency on babel-core, babylon. generate), if no, you'll benefit for any fixes I make in the future. You can of course fork it at any point in the future.

MatAtBread commented 6 years ago

Finally got around to forking Babel and adding the new transform (see https://github.com/MatAtBread/babel/tree/nodent-transform-async/packages/babel-plugin-transform-async-to-promises)

I'm having an issue with the name resolution:

matt@vb-linuxlite:~/git/babel$ echo 'async function x(){}' | ./packages/babel-cli/bin/babel.js --plugins ./packages/babel-plugin-transform-async-to-promises
function x() {
  return new Promise(function ($return, $error) {
    return $return();
  });
}

....works, but

matt@vb-linuxlite:~/git/babel$ echo 'async function x(){}' | ./packages/babel-cli/bin/babel.js --plugins @babel/transform-async-to-promises

{ Error: [BABEL] unknown: Cannot find module '@babel/plugin-transform-async-to-promises' from '/home/matt/git/babel'
    at Function.module.exports [as sync] (/home/matt/git/babel/packages/babel-core/node_modules/resolve/lib/sync.js:40:15)
    at resolveStandardizedName (/home/matt/git/babel/packages/babel-core/lib/config/loading/files/plugins.js:78:29)
    at resolvePlugin (/home/matt/git/babel/packages/babel-core/lib/config/loading/files/plugins.js:27:10)
    at loadPlugin (/home/matt/git/babel/packages/babel-core/lib/config/loading/files/plugins.js:35:18)
    at createDescriptor (/home/matt/git/babel/packages/babel-core/lib/config/option-manager.js:443:21)
    at /home/matt/git/babel/packages/babel-core/lib/config/option-manager.js:166:12
    at Array.map (<anonymous>)
    at /home/matt/git/babel/packages/babel-core/lib/config/option-manager.js:165:48
    at cachedFunction (/home/matt/git/babel/packages/babel-core/lib/config/caching.js:40:17)
    at /home/matt/git/babel/packages/babel-core/lib/config/option-manager.js:90:14 code: 'MODULE_NOT_FOUND' }

...does not (it does with transform-async-to-generators). Is there some other reference to the plugin name I'm not aware of that makes the path resolution work?

MatAtBread commented 6 years ago

@hzoo - Any ideas on the issue above ^^ ?

hzoo commented 6 years ago

Hmm it should link correctly.. although if the other one works I would be ok with PRing the existing plugin and renaming it to transform-async-functions

Oh well the folder name is hyphened and it's fine. It should be ok with the test format so no need to worry?

hzoo commented 6 years ago

I would just make the pr and we can work from there?

MatAtBread commented 6 years ago

@hzoo - here you go https://github.com/babel/babel/pull/7076

...let's continue the conversation there.