MatAtBread / fast-async

605 stars 21 forks source link

[bug] Errors when transforming code with for-of loops #55

Closed swernerx closed 6 years ago

swernerx commented 6 years ago

Hi!

Seems that some of the more recent version introduces errors during processing. Either babel-v6 is erroring out with unknown node of type "Literal" with constructor "Object" or there are warnings by nodent-transform: arboriculture.js:2327: Duplicate declaration '_iterator = _isArray ? _iterator : _iterator[Symbol.iterator]()'

Simple async code seems to work while it breaks with for-of-loops (works with "normal" for-in loops) and try-catch blocks. This is my reduced example which throws:

async function executeTasks(tasks) {
  for (const taskName of tasks) {
    try {
      await executeCommands()
    } catch (error) {
      console.error("Error")
    }
  }
}
swernerx commented 6 years ago

I am using babel-preset-env... so depending on browser/output scenarios there are a few differences.

I just pushed a new breaking test to my babel-preset for your reference: https://github.com/sebastian-software/babel-preset-edge/commit/1e97ac99293635e0dfb7863fb3872c054f68ef91

matAtWork commented 6 years ago

I shipped a change a few minutes ago which makes me suspicious, however I try your loop in the nodent online transform I don't have an issue (see the link below)

Are you trying to transform nodent (or fast-async) itself with Babel? This shouldn't be necessary as arboriculture is the transformer, not the generated code.

Please confirm you're using the correct version of fast-async for your version of Babel, fast-async@6 for Babel 6, fast-async@7 for Babel 7-Beta.

http://nodent.mailed.me.uk/#async%20function%20executeCommands(x)%20%7B%0A%20%20%20%20log(x)%3B%0A%20%20%20%20if%20(x%20%3D%3D%3D%20%22brown%22)%20%0A%20%20%20%20%20%20%20%20throw%20new%20Error(%22Brown%3F%22)%3B%0A%7D%0A%0Aasync%20function%20executeTasks(tasks)%20%7B%0A%20%20%20%20for%20(const%20taskName%20of%20tasks)%20%7B%0A%20%20%20%20%20%20%20%20try%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20await%20executeCommands(taskName)%3B%0A%20%20%20%20%20%20%20%20%7D%20catch%20(error)%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20console.error(%22Error%22)%3B%0A%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%7D%0A%0Aawait%20executeTasks(%22The%20quick%20brown%20fox%22.split(%22%20%22))%3B%0A~options~%7B%22mode%22%3A%22promises%22%2C%22promiseType%22%3A%22Zousan%22%2C%22noRuntime%22%3Atrue%2C%22es6target%22%3Afalse%2C%22wrapAwait%22%3Atrue%2C%22spec%22%3Atrue%7D

swernerx commented 6 years ago

I have a custom babel preset which uses "fast-async" as part of the plugin chain.

As you can see here in the package.json all versions point to v6: https://github.com/sebastian-software/babel-preset-edge/blob/master/package.json#L81 (babel+fast-async)

swernerx commented 6 years ago

I have played with the new "native" support for async-to-promises from this PR.: https://github.com/babel/babel/pull/7076 - that's a great milestone! Congratulations!

The error I experienced with using fast-async is also gone.

As mentioned here: https://github.com/babel/babel/pull/7076/files#r183963483 there seems to be a minor issue with logging though. Setting this to console.log I actually see a warning:

esnext_async_loop.js: Duplicate declaration '_iterator = _isArray ? _iterator : _iterator[Symbol.iterator]()'

As mentioned before, this is the code which I tested:

async function executeTasks(tasks) {
  for (const taskName of tasks) {
    try {
      await executeCommands()
    } catch (error) {
      console.error("Error")
    }
  }
}

The generated result looks like:

import "core-js/modules/es7.symbol.async-iterator";
import "core-js/modules/es6.symbol";
import "core-js/modules/web.dom.iterable";

function executeTasks(tasks) {
  return new Promise(function ($return, $error) {
    var _iterator, _isArray, _i, _ref;

    _iterator = tasks, _isArray = Array.isArray(_iterator), _i = 0, _iterator = _isArray ? _iterator : _iterator[Symbol.iterator]();
    var $Loop_2_trampoline;
    return ($Loop_2_trampoline = function (q) {
      while (q) {
        if (q.then) return void q.then($Loop_2_trampoline, $error);

        try {
          if (q.pop) {
            if (q.length) return q.pop() ? $Loop_2_exit.call(this) : q;else q = $Loop_2;
          } else q = q.call(this);
        } catch (_exception) {
          return $error(_exception);
        }
      }
    }.bind(this))($Loop_2);

    function $Loop_2() {
      if (true) {
        if (_isArray) {
          if (_i >= _iterator.length) return [1];
          _ref = _iterator[_i++];
        } else {
          _i = _iterator.next();
          if (_i.done) return [1];
          _ref = _i.value;
        }

        var $Try_1_Post = function () {
          try {
            return $Loop_2;
          } catch ($boundEx) {
            return $error($boundEx);
          }
        };

        var $Try_1_Catch = function (error) {
          try {
            console.error(\\"Error\\");
            return $Try_1_Post();
          } catch ($boundEx) {
            return $error($boundEx);
          }
        };

        try {
          return Promise.resolve(executeCommands()).then(function ($await_4) {
            try {
              return $Try_1_Post();
            } catch ($boundEx) {
              return $Try_1_Catch($boundEx);
            }
          }, $Try_1_Catch);
        } catch (error) {
          $Try_1_Catch(error)
        }
      } else return [1];
    }

    function $Loop_2_exit() {
      return $return();
    }
  });
}
swernerx commented 6 years ago

It looks like the code which is warned about by your generator is actually the result of transpiling es2015 for-of:

Input:

for (const taskName of tasks) {
  try {
    executeCommands()
  } catch (error) {
    console.error("Error")
  }
}

Output:

import "core-js/modules/es7.symbol.async-iterator";
import "core-js/modules/es6.symbol";
import "core-js/modules/web.dom.iterable";

for (var _iterator = tasks, _isArray = Array.isArray(_iterator), _i = 0, _iterator = _isArray ? _iterator : _iterator[Symbol.iterator]();;) {
  var _ref;

  if (_isArray) {
    if (_i >= _iterator.length) break;
    _ref = _iterator[_i++];
  } else {
    _i = _iterator.next();
    if (_i.done) break;
    _ref = _i.value;
  }

  try {
    executeCommands();
  } catch (error) {
    console.error("Error");
  }
}
MatAtBread commented 6 years ago

Ok, that's pretty interesting - the failure seems to come from transpiling the output of Bable's translation of for...of, is that correct?

MatAtBread commented 6 years ago

The logger not being a function is a straight bug. I'll patch that ASAP.

swernerx commented 6 years ago

I think that's right. I opened a bug report https://github.com/babel/babel/issues/7801

MatAtBread commented 6 years ago

Babel 6, 7 or both?

swernerx commented 6 years ago

Just Babel v7

MatAtBread commented 6 years ago

Hmmm. If I put your sample above into babel-7's cli, I get...

$ git/babel/packages/babel-cli/bin/babel.js --no-babelrc --presets=module:./git/babel/packages/babel-preset-env a.js 
"use strict";

var _iteratorNormalCompletion = true;
var _didIteratorError = false;
var _iteratorError = undefined;

try {
  for (var _iterator = tasks[Symbol.iterator](), _step; !(_iteratorNormalCompletion = (_step = _iterator.next()).done); _iteratorNormalCompletion = true) {
    var taskName = _step.value;

    try {
      executeCommands();
    } catch (error) {
      console.error("Error");
    }
  }
} catch (err) {
  _didIteratorError = true;
  _iteratorError = err;
} finally {
  try {
    if (!_iteratorNormalCompletion && _iterator.return != null) {
      _iterator.return();
    }
  } finally {
    if (_didIteratorError) {
      throw _iteratorError;
    }
  }
}

...and if I put that into 'nodent's CLI (which should log all the outpit to stdout) I get:

$ git/babel/packages/babel-cli/bin/babel.js --no-babelrc --presets=module:./git/babel/packages/babel-preset-env a.js | git/nodent/nodent.js --out --use=promises
"use strict";
var _iteratorNormalCompletion = true;
var _didIteratorError = false;
var _iteratorError = undefined;
try {
    for (var _iterator = tasks[Symbol.iterator](), _step;!(_iteratorNormalCompletion = (_step = _iterator.next()).done); _iteratorNormalCompletion = true) {
        var taskName = _step.value;
        try {
            executeCommands();
        } catch (error) {
            console.error("Error");
        }
    }
} catch (err) {
    _didIteratorError = true;
    _iteratorError = err;
} finally {
    try {
        if (!_iteratorNormalCompletion && _iterator.return != null) {
            _iterator.return();
        }
    } finally {
        if (_didIteratorError) {
            throw _iteratorError;
        }
    }
}

...which looks correct (unsurprisingly, as with any async or await keywords the transform won't do anything)

swernerx commented 6 years ago

Mhh, the whole stuff it tricky. I now see your results as well. When using a clean non babel v6 environment. It seems like Babel v7 is relatively unstable when some Babel v6 stuff is in node_modules. Not yet sure, why, though.

matAtWork commented 6 years ago

Can I suggest change the order of your plugins - try with fast-async first and again with it last to see if it makes any difference. I'm guessing that either Babel is producing code that fast-async can't transform, or the other way around. But I'd like to know!

MatAtBread commented 6 years ago

FYI, the related issue of logger: false has now been patched in nodent-transform

swernerx commented 6 years ago

I figure I close this one as I don't see currently any issue. But I have a new one :)