babel / babel

🐠 Babel is a compiler for writing next generation JavaScript.
https://babel.dev
MIT License
42.99k stars 5.59k forks source link

Implement transform-async-to-promises #7076

Open MatAtBread opened 6 years ago

babel-bot commented 6 years ago

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7756/

hzoo commented 6 years ago

Great thanks for the initial PR, are you ok if I make some commits? I think this has tests from the other transforms, so we should merge into the existing plugin (which I can do if it was erroring) or at least remove the old files.

MatAtBread commented 6 years ago

@hzoo - as requested, pull implementing transform-async-to-promises.

As I understand it, you're intending to merge into transform-async-to-generator and used options to specify the target.

I have implemented only two options:

MatAtBread commented 6 years ago

...sure - it's your project/plugin. Like everyone I have an opinion, but I'm no Babel expert (and only a very occasional user). Feel free to ask questions, raise issues, etc. I'm certainly interested in where it goes next

matAtWork commented 6 years ago

@hzoo - I have no idea how to finally get this pull accepted. Maybe you can help?

matAtWork commented 6 years ago

Is anyone able to explain the CircleCI issues? They appear unrelated to the changes in the pull Thanks.

matAtWork commented 6 years ago

@hzoo - are you able to help in resolving these checks?

matAtWork commented 6 years ago

Do I need to do anything to get this merged?

ljharb commented 6 years ago

I’ll ping some folks to take a look at it.

matAtWork commented 6 years ago

Ping....

matAtWork commented 6 years ago

@ljharb @hzoo - any update on the status of this PR? I've been asked to break out the relevant bits to a new (external) plugin, but I'd much rather tell people to use the "official" one.

ljharb commented 6 years ago

I’ll ping again in the babel slack; I’m not comfortable merging it alone, but i think it’s ready.

hzoo commented 6 years ago

Apologies again Matt, been really busy with trying to just get Babel v7 released and other personal stuff now that I've left my job. I'll try but maybe someone can look into this atm?

xtuc commented 6 years ago

I'm just wondering, this new plugin doesn't require to be in the Babel monorepo. I would just mention it in the doc.

ljharb commented 6 years ago

@xtuc it's pretty important i think that it is; the regenerator transform needs to not be the default one day, and this is an important stepping stone in that direction.

loganfsmyth commented 6 years ago

~Could someone clarify, does this plugin completely annihilate sourcemap support? It seems like this serialize and re-parse approach would render sourcemaps useless. I see nodent has a sourcemap option, but it doesn't look like that's accounted for, and I have no sense of if that would be doable in our cases.~

~There hasn't been a ton of movement on this because several of us are concerned above the approach of nodent's requirement to serialize and re-parse blocks it from moving toward an officially-supported plugin. As it stands, it's something that no-one on the Babel team could actually help maintain, and it's a huge step away from how any of our existing plugins are implemented.~

~@ljharb I'm certainly not against us avoiding regenerator usage for async functions, but as it is this plugin doesn't seem like something I'd actually want us to promote. To me, at least, it feels like it would need to be a real AST transform before I'd feel comfortable with it being the default.~

Ignore this, I had some totally wrong assumptions.

MatAtBread commented 6 years ago

@loganfsmyth - the implementation here (nodent-transform) doesn't serialize or re-parse. It simply manipulates the AST, preserving location information where possible.

MatAtBread commented 6 years ago

it would need to be a real AST transform

In what way is it not a real AST transform?

loganfsmyth commented 6 years ago

@MatAtBread Oh! Well that's my misunderstanding then, sorry about that. I'll see if I can find some time for a more in-depth review so I don't make other incorrect comments.

MatAtBread commented 6 years ago

I'm happy to answer any queries you might have. This re-work was started following a conversation with @hzoo last year. The nodent-transform was refactored out of the compiler/cli code, and has no external dependencies (eg no acorn or code-generator).

However, to allow me to only have to maintain one core transform (at least for now), it doesn't use the Babel AST helpers (or anyone elses) and is monolithic, which, from your PoV, would make it look big and with some unnecessary helpers (eg: a tree walker, JS templates).

MatAtBread commented 6 years ago

If anyone has any hints as to why Travis failed (all I did was update the README!), I'd appreciate some input.

hzoo commented 6 years ago

Maybe needs a rebase? can use [skip ci] for the readme change later

MatAtBread commented 6 years ago

Is it just me, or is the current master (beta.46) failing tests?

swernerx commented 6 years ago

Possibly someone should check this related issue: https://github.com/babel/babel/issues/7815

ljharb commented 6 years ago

@xtuc whoa, this has been discussed for a long time. The maintenance cost comes from having a default transform to generators; this transform moves towards avoiding/reducing that cost. Can you elaborate more on why you think this shouldn’t be in babel? The airbnb config, and all its users, are unable to use async/await without this transform, and if it’s not in babel core, the maintenance cost of the transform itself gets much higher.

WaldoJeffers commented 6 years ago

@ljharb I 100% agree with you. If I am not mistaken, this would help solve many errors linked to babel-regenerator, which are legion (at least on Node v >4, since it natively support Promises)

green-pickle commented 6 years ago

@xtuc wtf man? There are lots of devs waiting for this, enormous amount of issues with the default generators transform. Why do we need to suffer and learn another shit ton of config just because you are scared of maintenance cost?

The PR was written by the community (thanks a lot @MatAtBread) and with the help of the rest of the community I'm pretty sure it will be maintained well.

Just don't be that guy, please.

nicolo-ribaudo commented 6 years ago

I agree with @xtuc. From my point of view, the main problem is that this PR doesn't implement the transform, but it is just a wrapper around nodent.

@sexualmaniac If you want so much to use this functionality, you can use https://github.com/MatAtBread/fast-async. If you look at the readme it is not "another shit ton of config", it is ONE line in your babel config and ONE line in your package.json. @xtuc and every other team member is already doing much work, so please don't be so harsh.

ljharb commented 6 years ago

@nicolo-ribaudo it's a wrapper around a piece of nodent, not around all of nodent - @MatAtBread took the time and did the work to extract out that piece so that both nodent and babel could use it without needing to take on an otherwise large dependency.

The longer term plan is to avoid the nodent-transform dependency entirely, but the first step was going to be to get this transform added, with tests, so @MatAtBread could step back and others could build on that work.

ljharb commented 6 years ago

The real issue is that depending on regenerator in the first place for async/await was a very very unfortunate decision that has had real maintenance costs, and it's critical for babel and babel users that there be a promises transform.

It certainly doesn't have to be this implementation or this transform, but objecting to this implementation with concerns about maintenance cost seems in bad faith unless someone's volunteering to do the work themselves to build an "async to promises" transform that would be considered acceptable. Is anyone volunteering to do that?

green-pickle commented 6 years ago

@nicolo-ribaudo I absolutely love babel and the work of the team behind it.

This is not one or two lines, this is - read readme, configure, add runtime, test if it works, tell everyone why we need this. And we have other things, you know, like webpack with the whole world of loaders and configs.

Promises already have good support, let's make devs life easier with this change so we can get rid of regenerator once and forever.

matAtWork commented 6 years ago

Is there a way to move forward with this? Or is it now blocked...

hzoo commented 6 years ago

You don't need to anything @matAtWork, we just need to figure out what to do next. Again, for anyone else you can just use fast-async normally which works fine on its own.

shellscape commented 6 years ago

I'd like to see this happen. fast-async works great but increases the size of some of my files by a factor of 4. Which is just 🍌

ljharb commented 6 years ago

I prefer 10 times larger files even, over the runtime cost of using regenerator - but i want both plugins in core so that people are able to make that choice.

Dan503 commented 6 years ago

I am so glad that this pull request is so close to making it's way into Babel core. :D

With current Babel functionality, in order to use async functions with preset-env in IE11 the entire babel-polyfill plugin needs to be downloaded by not just authors but also users just so that regeneratorRuntime is not undefined (a function that only Babel uses by the way).

This gives an idea of the sort of impact that the download alone has on user experience: https://bundlephobia.com/result?p=babel-polyfill@6.26.0

By switching to a promise based async translation, the only thing it needs to work is a Promise polyfill. Promises are things that developers are able to take advantage of in their regular source code. This makes a Promise polyfill a far more worth while investment than all the code necessary to make regeneratorRuntime work which developers don't use at all in their source code. Promises are also a native browser API so only the browsers that don't understand Promises have to download and parse the polyfill. Smart devs can prevent modern browsers from downloading the polyfill code all together. regeneratorRuntime is non-standard code so all browsers, no matter how cutting edge they are, have to download and parse the entire babel-polyfill library no matter what.

On top of that, every single time you include an async function it massively blows out the code size.

For example take this basic bit of js:

var that = async function(){
    var thing = await fetch('https://dog.ceo/api/breeds/image/random').then(resp => resp.json());
    return thing;
}
var result = that();
console.log(result);

It will get blown out into this monster:


var that = function that() {
  var thing;
  return regeneratorRuntime.async(function that$(context$2$0) {
    while (1) switch (context$2$0.prev = context$2$0.next) {
      case 0:
        context$2$0.next = 2;
        return regeneratorRuntime.awrap(fetch('https://dog.ceo/api/breeds/image/random').then(function (resp) {
          return resp.json();
        }));

      case 2:
        thing = context$2$0.sent;
        return context$2$0.abrupt('return', thing);

      case 5:
      case 'end':
        return context$2$0.stop();
    }
  }, null, this);
};
var result = that();
console.log(result);

By switching to a promise based translation the code can be simplified down to this (based on the output from babel-plugin-async-to-promises).

var that = function that() {
  var thing;
  return Promise.resolve().then(function () {
    return fetch('https://dog.ceo/api/breeds/image/random').then(function (resp) {
      return resp.json();
    });
  }).then(function (_resp) {
    thing = _resp;
    return thing;
  });
};
var result = that();
console.log(result);

This seems like the far more logical approach. The output is more human readable and also creates far less code bloat.

(The plugin doesn't seem to override the preset-env functionality so I can't use it if env is the preset. It was built to run on the es2015 preset.)

Please merge this pull request into Babel core. The current default translation is madness!

benjamn commented 6 years ago

Disclaimer: I'm the original author and current maintainer of Regenerator.

I prefer 10 times larger files even, over the runtime cost of using regenerator - but i want both plugins in core so that people are able to make that choice.

@ljharb Can you quantify that claim?

Are you just talking about the size of regenerator-runtime? Because it's 2.8KB after minification and gzip, which is noticeable for sure, but nodent-runtime is 1.7KB, so it's not as if this transform has no runtime library. It's not even especially tiny.

The costs of both of these static assets are constant, and will be amortized over time by browser caching, as I'm sure you would argue if you were trying to justify your own additions to client bundle size. And you'd be right!

I will assume you're not talking about compilation performance, since you said "runtime cost," and compilation performance isn't nearly as important as runtime performance.

So... do you have anything to say about the relative runtime performance of fast-async/nodent code versus code generated by Regenerator?

Because I sure do!

Here's a test program that I just wrote:

require("nodent-runtime");
const regeneratorRuntime = require("regenerator-runtime");

async function eachAsync(items, fn) {
  let sum = 0;
  const count = items.length;
  for (let i = 0; i < count; ++i) {
    sum += await fn(items[i]);
  }
  return sum;
}

const items = [];
for (let i = 0; i < 100000; ++i) {
  items.push(i);
}

const start = +new Date;
eachAsync(items, item => item).then(sum => {
  console.log("sum", sum);
  console.log(new Date - start, "ms");
}, error => {
  console.log(error);
});

Here's the code Regenerator generates:

require("nodent-runtime");

var regeneratorRuntime = require("regenerator-runtime");

function eachAsync(items, fn) {
  var sum, count, i;
  return regeneratorRuntime.async(function eachAsync$(_context) {
    while (1) {
      switch (_context.prev = _context.next) {
        case 0:
          sum = 0;
          count = items.length;
          i = 0;

        case 3:
          if (!(i < count)) {
            _context.next = 10;
            break;
          }

          _context.next = 6;
          return regeneratorRuntime.awrap(fn(items[i]));

        case 6:
          sum += _context.sent;

        case 7:
          ++i;
          _context.next = 3;
          break;

        case 10:
          return _context.abrupt("return", sum);

        case 11:
        case "end":
          return _context.stop();
      }
    }
  }, null, this);
}

var items = [];

for (var i = 0; i < 100000; ++i) {
  items.push(i);
}

var start = +new Date();
eachAsync(items, function (item) {
  return item;
}).then(function (sum) {
  console.log("sum", sum);
  console.log(new Date() - start, "ms");
}, function (error) {
  console.log(error);
});

I'm honestly not sure how to evaluate accusations that this code is ugly. It's not what you wrote, by any stretch of the imagination, but neither is the code generated by fast-async or nodent. Debuggability kinda goes out the window when you're transpiling async functions, which is why it's especially important to have some sort of source maps, which is a whole lot easier if your transform is implemented as a Babel plugin, as Regenerator is.

Now for the nodent code:

require("nodent-runtime");
const regeneratorRuntime = require("regenerator-runtime");
function eachAsync(items, fn) {
    return (function ($return, $error) {
        let sum, count;
        sum = 0;
        count = items.length;
        var $Loop_1_local;
        function $Loop_1_step() {
            var [i] = $Loop_1_local();
            ++i;
            return $Loop_1.bind(this, i);
        }

        function $Loop_1(i) {
            $Loop_1_local = function () {
                return [i];
            };
            if (i < count) {
                return fn(items[i]).then((function ($await_3) {
                    sum += $await_3;
                    return $Loop_1_step;
                }).$asyncbind(this, $error), $error);
            } else 
                return [1];
        }

        return Function.$asyncbind.trampoline(this, $Loop_1_exit, $Loop_1_step, $error, true)($Loop_1.bind(this, 0));
        function $Loop_1_exit() {
            return $return(sum);
        }

    }).$asyncbind(this, true);
}

const items = [];
for (let i = 0;i < 100000; ++i) {
    items.push(i);
}
const start = +new Date();
eachAsync(items, item => item).then(sum => {
    console.log("sum", sum);
    console.log(new Date() - start, "ms");
}, error => {
    console.log(error);
});

Ok, come on, are we seriously spending time debating the relative beauty of these two walls of unreadable code?

Yes, it turns out in this one case that Regenerator's code is shorter, but I don't even care about that. Minified and gzip'd, one of them is 350 bytes and the other is 357 bytes. That's a wash, folks.

What I care about, and what Regenerator implicitly "cares" about, is runtime performance of the generated code.

Here's the output of this program for Regenerator:

> npx regenerator perf.js > perf.regenerator.js
> node perf.regenerator.js
sum 4999950000
46 'ms'

46 milliseconds to sum 100,000 numbers. I would say "not bad," but how would I know? Let's compare to native async/await:

> node --version
v8.11.2
> node perf.js
sum 4999950000
33 'ms'

Cool, the Regenerator code took only 1.4x longer than native. I'll take it.

And now here's the output of the same program as compiled by nodentjs:

> npx nodentjs --out perf.js > perf.promises.js
> node perf.promises.js
TypeError: fn(...).then is not a function
    at $Loop_1 (/Users/ben/dev/babel/packages/babel-plugin-transform-async-to-promises/test/fixtures/loops/perf.promises.js:20:37)
    at b (eval at processIncludes (/Users/ben/dev/babel/packages/babel-plugin-transform-async-to-promises/test/fixtures/loops/node_modules/nodent-runtime/runtime.js:36:12), <anonymous>:3:486)
    at /Users/ben/dev/babel/packages/babel-plugin-transform-async-to-promises/test/fixtures/loops/perf.promises.js:28:94
    at boundThen (eval at processIncludes (/Users/ben/dev/babel/packages/babel-plugin-transform-async-to-promises/test/fixtures/loops/node_modules/nodent-runtime/runtime.js:36:12), <anonymous>:3:4435)
    at new Zousan (eval at processIncludes (/Users/ben/dev/babel/packages/babel-plugin-transform-async-to-promises/test/fixtures/loops/node_modules/nodent-runtime/runtime.js:36:12), <anonymous>:3:2373)
    at Function.$asyncbind (eval at processIncludes (/Users/ben/dev/babel/packages/babel-plugin-transform-async-to-promises/test/fixtures/loops/node_modules/nodent-runtime/runtime.js:36:12), <anonymous>:3:4518)
    at eachAsync (/Users/ben/dev/babel/packages/babel-plugin-transform-async-to-promises/test/fixtures/loops/perf.promises.js:33:8)
    at Object.<anonymous> (/Users/ben/dev/babel/packages/babel-plugin-transform-async-to-promises/test/fixtures/loops/perf.promises.js:41:1)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)

Okay, this is my first time using nodentjs. Maybe I goofed something up. Can you spot the problem in the code above? Is it something trivial or fundamental?

Have a closer look at these lines:

return fn(items[i]).then((function ($await_3) {
    sum += $await_3;
    return $Loop_1_step;
}).$asyncbind(this, $error), $error);

This comes from the following original code:

sum += await fn(items[i]);

The nodentjs code seems to be assuming that any awaited argument must be a thenable object (like a Promise), but of course that's not necessarily true (that's one of the great things about await—it doesn't care), which is why Regenerator uses a runtime helper:

  _context.next = 6;
  return regeneratorRuntime.awrap(fn(items[i]));

case 6:
  sum += _context.sent;

I don't mean to belabor this bug, but I will say it makes me wonder what other basic async/await functionality is not up to spec. I would also imagine this would bother @ljharb if he found out about it.

Okay, obviously I can fix the generated code by hand:

return Promise.resolve(fn(items[i])).then((function ($await_3) {
    sum += $await_3;
    return $Loop_1_step;
}).$asyncbind(this, $error), $error);

Now, the code runs without an error, and we can finally see how runtime performance compares:

> node perf.promises.js
sum 4999950000
83 'ms'

1.8x longer than Regenerator's code, and 2.5x longer than native.

Look, folks, this is a microbenchmark for sure, and we've all happily used programming languages that execute code significantly slower than the equivalent C code, but I just don't see what anyone is talking about when they claim there is a runtime performance disadvantage to using Regenerator.

@ljharb I'll ask again. What on earth are you talking about?

shellscape commented 6 years ago

holy wow batman

ljharb commented 6 years ago

@benjamn indeed I’m talking about runtime performance, but as Airbnb will likely be eternally forbidding await in loops, that’s not a representative example of the code I’m hoping to transform. How do the two compare with examples that don’t actually require a finite state machine to transpile? (of course, this is all off topic for this PR, since this PR is about adding a transform option to babel, and is simply not the appropriate place at all to debate regenerator-runtime).

Good catch on that bug - @MatAtBread can we update this PR to address the fact that all values, including null/undefined and non-thenables, are awaitable?

matAtWork commented 6 years ago

The assumption await is followed by a Promise is a documented assumption here - if you can make this assumption in (some of) your code, it's a significant optimisation.

The "fast-async@6" implementation sets the wrapAwait flag if the spec: true option is provided to babel. "fast-async@7" sets it by default, so is entirely compliant with the spec by default - a look at the original discussion with @hzoo here would have shown that up. Additionally the noRuntime and noNodentExtensions compiler flags (which have the same defaults) removes any references and need for the runtime, at the expense of inlining code where necessary. Your choice to use the runtime or not is entirely personal - it has no effect on the execution semantics.

Given that @benjamn did indeed "goof up" as he suspected, I kind of object to the implication that nodent may or may not "secretly" miss other parts of the spec. Maybe reading the docs or asking might be nicer than innuendo? If someone does find an issue, just report it and I'll look into fixing it, like I always do.

Anyway, enough bitching. I can't comment on regenerator, as I don't know it. What I can tell you is an npm test in nodent compares the performance callbacks, promises, generators and the native async/await. The good news is the native implementions (at least in later V8 environments like Node >9) are the fastest. Generator performance is always the worst, becuase whether your generators are native or based on regenerator (which is quite a bit slower that native generators), the turn-by-turn Promise allocation of the async runner in the TC-39 paper is very inefficient (it's a reference implementation - not performance/production optimized). It allocates Promises on every turn and chains them, even if there is no need (logically) to do so.

Finally, your choice of Promise library makes a lot of difference. I'm afraid bluebird is a perfect example of premature-optimization. The very things that made it fast a few years back (like calling eval to remove closure references by re-evaluating functions) now make it slow, and the native implementations have more than caught up. If you need a Promise polyfill my preference is for Zousan. I cannot overstate the differences these choices make, and how they vary from one release/engine to another.

As for readability, I find nodent's output reasonably easy to follow in most circumstances. That's mainly because I wrote it to do automatically what I was doing manually as a programmer. I'd agree with @benjamn that asynchronized loops are the hardest to follow. For a real mind-bending experience put a finally block in the loop body together with a conditional break or continue with a label - that can really hurt. Luckily source maps avoid most of the really hard work.

Having put some effort into getting this fit for use with Babel, I'd like to get it over the finish line - but I have no dog in this fight: I'm not a Babel user myself and in professionally I'm writing mainly server-side code and control the environment, so I can (and do) move to later Node releases obviating my need for nodent too.

matAtWork commented 6 years ago

For those that are interested. npm i nodent, then node --expose-gc ./nodent.js tests tests/semantics/perf*

Node v7 (earlier ones are similar)

                                                      {es7:true}          nodent          native
                  Compiler flags            Mean             173             380             660
               es7,lazyThenables             100             100               -               -
     es7,lazyThenables,es6target             106             106               -               -
     es7,lazyThenables,wrapAwait             189             189               -               -
azyThenables,es6target,wrapAwait             202             202               -               -
                             es7             187             187               -               -
                   es7,es6target             182             182               -               -
                   es7,wrapAwait             207             207               -               -
         es7,es6target,wrapAwait             210             210               -               -
                        promises             334               -             155             513
              promises,es6target             334               -             158             510
              promises,noRuntime             307               -             136             477
    promises,es6target,noRuntime             306               -             134             479
              promises,wrapAwait             347               -             183             512
    promises,es6target,wrapAwait             356               -             184             528
    promises,noRuntime,wrapAwait             327               -             159             495
es,es6target,noRuntime,wrapAwait             329               -             160             497
                      generators             890               -             820             960
            generators,es6target             933               -             846            1020
            generators,wrapAwait             914               -             795            1033
  generators,es6target,wrapAwait             861               -             831             892

Node v8 (and later)

                  Compiler flags            Mean               -             127             134
               es7,lazyThenables               -               -               -               -
     es7,lazyThenables,es6target               -               -               -               -
     es7,lazyThenables,wrapAwait               -               -               -               -
azyThenables,es6target,wrapAwait               -               -               -               -
                             es7               -               -               -               -
                   es7,es6target               -               -               -               -
                   es7,wrapAwait               -               -               -               -
         es7,es6target,wrapAwait               -               -               -               -
                        promises             106               -             100             112
              promises,es6target             109               -             101             116
              promises,noRuntime              94               -              86             101
    promises,es6target,noRuntime              94               -              86             103
              promises,wrapAwait             113               -             106             120
    promises,es6target,wrapAwait             112               -             106             118
    promises,noRuntime,wrapAwait              95               -              90             101
es,es6target,noRuntime,wrapAwait              97               -              90             104
                      generators             370               -             373             367
            generators,es6target             341               -             335             347
            generators,wrapAwait             361               -             364             357
  generators,es6target,wrapAwait             344               -             324             364
                          engine              47               -              47              48
                engine,es6target              47               -              47              48
                engine,noRuntime              48               -              48              47
      engine,es6target,noRuntime              47               -              47              48
                engine,wrapAwait              47               -              47              47
      engine,es6target,wrapAwait              47               -              48              47
      engine,noRuntime,wrapAwait              47               -              47              47
ne,es6target,noRuntime,wrapAwait              47               -              48              47

Note in Node v8, the "callbacks" method runs out of stack and fails. It's also heavily deprecated and you have to set all kinds of flags to resurrect in nodent. By node v10 native Promise performance has surpassed Zousan, but the native impl is twice as quick. Generators remain >3x times slower - it's just the way they're made. The test routines are simple loops (perf and perf-2) from here

shellscape commented 6 years ago

Has a sense of humor been banned from this repo? Good lord folks, a meme is not spam. Especially when it's directly related to the previous reply, clean, and non-offensive.

benjamn commented 6 years ago

Thanks for the thoughtful feedback, @ljharb and @MatAtBread.

Before I respond in more detail, I need to say something specifically to @MatAtBread:

I do love the idea of this transform. It's totally different from anything I imagined when I was implementing Regenerator, in part because I was focused on generator functions, and async/await was still just a thing C# programmers raved about. I’m amazed that your technique works so well, and I am genuinely glad you decided to pursue this alternative approach, because pushing the limits of transpilation is fascinating to me, and that's exactly what you're doing. I would love to chat about this stuff with you any time.

Additionally the noRuntime and noNodentExtensions compiler flags (which have the same defaults) removes any references and need for the runtime, at the expense of inlining code where necessary. Your choice to use the runtime or not is entirely personal - it has no effect on the execution semantics.

This is good to know, but the runtime is enabled by default with good reason: it tends to pay for itself, because compiling without the runtime produces more code:

% npx nodentjs --out perf.js | npx uglify-es -m -c | gzip | wc -c
     339
% npx nodentjs --out --noruntime --use=promises perf.js | npx uglify-es -m -c | gzip | wc -c
     400

With ~18% more code per async function (note: I'm sure this percentage varies), it doesn't take many async functions before the runtime begins making your bundle smaller. And at that point we might as well assume the runtime is a fact of life in most applications.

Again, I am not faulting nodent for these choices, but I reject them as a basis for favorable comparison to Regenerator.

i want both plugins in core so that people are able to make that choice … this PR is about adding a transform option to babel

@ljharb This plugin does not need to be included in the @babel/ namespace in order for developers to have that choice.

Inclusion in core has less to do with enabling developer choice, and more to do with these two (real!) benefits:

How many times have you claimed (to me and others) that developers don't know what's best for them, as a way of stressing the importance of spec compliance over developer choice?

We have slightly different priorities, you and I, but I think we can find a common ground. I firmly believe developers deserve to make informed choices, and I think you would too, provided those choices are truly "informed." If you agree with that, then I'm sure you can also agree they deserve to hear all the facts (more on this in my forthcoming review comments).

The truth is, developers who use this plugin will probably also enable Regenerator, so that generator functions (and async generator functions) will be supported as well as async functions. If they don't actually use any generator functions (perhaps because their linter forbids them), Regenerator will have nothing to compile, and regenerator-runtime will not be included. 🍰

benjamn commented 6 years ago

@shellscape Take the compliment!

matAtWork commented 6 years ago

@benjamn - I should improve the README, however the spec-compliance bit is moot. I referenced that to demonstrate I had been aware of (and provided options for) this for many, many months. The plugin as provided in "fast-async@7" (and the PR) has all these compliance options on by default, so there is no need to reference the README to make sure developers are:

especially [informed of] the spec-compliance caveats

...since there aren't any (to the best of my knowledge).

The default config for the Babel plugin (not the CLI, which has nothing to do with Babel) does accept await nonPromiseValue as specified (see here), with some performance overhead. As far as I am aware it is fully spec-compliant.

Inclusion of the runtime is already provided for as a Babel plugin option here. The default is inlining, as requested by @hzoo

The issue about porting to the Babel API to use the pipelining or whatever (since nodent-transform is standalone and contains a small set of it's own AST utilities) is real. The port, AFAIK is not quite a one-2-one renaming/substitution and might be tricky in places. The whole transform is complex (especially loops and exception handlers) and multi-pass which might not play nicely with the Babel way of doing things.

Again, I have no agenda here, beyond not wanting to waste the effort I've already put in to making this "Babel Ready". Maybe I should have asked more widely what that meant.

matAtWork commented 6 years ago

....also, I agree with you about tests. The tests in nodent-compiler are all execution tests which exercise a code path or represent an issue raised, fixed and closed.

I believe the test set in Babel was copied from the existing async-to-generators package with the expected code generated by the plugin? @hzoo very kindly did this after my initial PR as I was a bit lost as to how the tests hung together.

matAtWork commented 6 years ago

@benjamn - while we're building bridges (since my face is now all over your twitter feed), I've never compared anything unfavourably to to regenerator myself, as I don't use it.

I have demonstrated above (and you can run for yourself in nodent's perf tests) that the TC-39 generator implementation of async/await is pretty sub-optimal. I have no idea what optimisations or improvements are in babel-transform-async-to-generator as I've not looked, so I make no claims about that, regenerator or it's runtime.

Please don't conflate other people's observations or opinions with my own. I totally agree with you about runtimes - I use nodent's runtime, but I was asked to make it optional for what, I assume, were good reasons by @hzoo (probably simplicity).

hzoo commented 6 years ago

I apologize that this thread/PR got so long and delayed. I too want to see an async to promise transform (why we tried with https://www.npmjs.com/package/babel-plugin-async-to-promises). But I understand the concerns of others regarding the plugin itself since most of the logic isn't really in babel at all so none of us would really be able to handle the changes. fast-async does work in the meantime and we've barely had enough bandwidth to release v7 so I haven't really been paying attention to this as much. I appreciate the tons of work @matAtWork already put in. I know that porting all the logic into a pure babel plugin might not be feasible and not doing it may make us hesitant to merge this in the first place if it could be a separate plugin outside the repo.

Thanks for the points @benjamn, I made assumptions when initially looking to implement this. The original async-to-promises transform was purely written with Babel so it was what I assumed as well for fast-async.

This transform really needs to be rewritten as a Babel plugin, as regenerator-transform has been I too have Babel-compatible plugins that aren't really Babel plugins, such as babel-plugin-transform-es2015-modules-reify, but I believe that should disqualify them from inclusion in core

The port, AFAIK is not quite a one-2-one renaming/substitution and might be tricky in places. The whole transform is complex (especially loops and exception handlers) and multi-pass which might not play nicely with the Babel way of doing things.

I understand that it might not be easy to do this with nodent though which is the issue despite the previous efforts.

Yeah, I don't believe we've had any babel plugins in "core" or the monorepo that just defered to another package that wasn't a babel plugin entirely itself. Based on this maybe it would make sense to just make it another package in the org or just update fast-async to v7? I believe this was the initial issue when looking at the implementation. So maybe just moving it under the org isn't much different than keeping it as fast-async for maintained there. I don't think anyone is opposed to the general idea of transforming async to promises though.

benjamn commented 6 years ago

while we're building bridges (since my face is now all over your twitter feed), I've never compared anything unfavourably to to regenerator myself, as I don't use it.

You're right, and I'm sorry. I have now deleted that tweet.

I have demonstrated above (and you can run for yourself in nodent's perf tests) that the TC-39 generator implementation of async/await is pretty sub-optimal. I have no idea what optimisations or improvements are in babel-transform-async-to-generator as I've not looked, so I make no claims about that, regenerator or it's runtime.

For what it's worth, Regenerator's implementation is not exactly "a generator implementation of async/await". Regenerator code can be faster than Promises + native generators (i.e. the TC39 reference implementation) because it compiles the generator functions and yield expressions down to ES5, which is often faster than native generators. JS engines have had many more years to figure out how to optimize switch statements.

Also, Regenerator can compile async functions directly to an ES5 state machine, without first creating a full generator function that will only be called once, which is typically what happens if you compile async functions to Promises and native generator functions, and then compile the generator functions, in two separate compilation phases.

In other words, as you acknowledged above, the only way to compare performance against Regenerator would be to run code generated by Regenerator. I'm not asking you to do that work, I just want it to be clear that Regenerator is not the reference implementation.

Please don't conflate other people's observations or opinions with my own.

Point taken. I will no longer assume that anyone else on this thread is speaking on your behalf.

matAtWork commented 6 years ago

@benjamn - thanks, it's appreciated.

Anytime you want to exchange insane AST manipulation stuff, I'm you new BFF. I might point you at a crazy repo I have which implements "concatenative JS", because the first compiler I wrote was a FORTH complier (in about 1984).