MatAtBread / fast-async

603 stars 21 forks source link

Await inside for loop does not work #13

Closed Rush closed 8 years ago

Rush commented 8 years ago
async liveBackup(files) {
  /* some other code here */
  for(let file of files) {
    await execFileAsync('touch', [backupFile(file.fullPath)]);
  }
}
{ TypeError: /code/app/lib/api/machines.js: Duplicate declaration "file" (This is an error on an internal node. Probably an internal error)
    at File.buildCodeFrameError (/code/app/node_modules/babel-core/lib/transformation/file/index.js:431:15)
    at Scope.checkBlockScopedCollisions (/code/app/node_modules/babel-traverse/lib/scope/index.js:397:27)
    at Scope.registerBinding (/code/app/node_modules/babel-traverse/lib/scope/index.js:579:16)
    at Scope.registerDeclaration (/code/app/node_modules/babel-traverse/lib/scope/index.js:483:14)
    at Object.BlockScoped (/code/app/node_modules/babel-traverse/lib/scope/index.js:185:28)

It works fine with babel's async-to-module-method.

matAtWork commented 8 years ago

It works for me (see link below). Could it be that async liveBackup should be async function liveBackup?

http://nodent.mailed.me.uk/#async%20function%20execFileAsync(op%2Cfile)%7B%0A%20%20%20%20%2F%2F%20Do%20something%0A%20%20%20%20console.log(op%2Cfile)%0A%7D%0A%0Aasync%20function%20liveBackup(files)%20%7B%0A%20%20%2F*%20some%20other%20code%20here%20*%2F%0A%20%20for(let%20file%20of%20files)%20%7B%0A%20%20%20%20await%20execFileAsync('touch'%2C%20%5Bfile%5D)%3B%0A%20%20%7D%0A%7D%0A%0Aawait%20liveBackup(%5B'a'%2C'b'%5D)

Please confirm and close or provide additional information.

Rush commented 8 years ago

This function is part of an object. I will try to make a small reproducible test-case.

matAtWork commented 8 years ago

Thanks. From the stack trace, it looks like it might be related to the let. If you can get something to work in the playground, but not in Babel, let me know as it might be an unpleasant interaction between Babel and Nodent.

Rush commented 8 years ago

@matAtWork ok, this the most minimal case that breaks:

async function liveBackup() {
  for(let file of files) {
    await Promise.resolve();
  }
  for(let file of files) {
    await Promise.resolve();
  }
}
npm install fast-async-issue-13
cd node_modules/fast-async-issue-13
npm test
matAtWork commented 8 years ago

Yep, that's a bug. Nodent is mangling the scope for declarations within a for-loop. I'll work on a fix. The only workaround I can suggest short-term is to use different variable names in your for loop declarations, or use 'var' since it has function scope.

Rush commented 8 years ago

I was merely curious about nodent. Using async to bluebird coroutine works fine. I will give it another shot once the bug is fixed though.

btw. will stacktraces show proper line numbers in nodent when combined with babel?

matAtWork commented 8 years ago

I doubt it. Nodent's stack trace mapping is a run-time feature, which I imagine Babel mangles as the sourcemaps generated by Nodent won't make through to execution. TBH, I've not tried - it might work depending on how transparent Babel is.

I've fixed the bug in Nodent v3 (not released yet), and almost back-ported it to v2.6.x but one of the main differences is how async loops are transpiled so it's a work in progress.

Rush commented 8 years ago

Nice, I'll try it out after the release. If you generate an inline source map then babel should pick it up.

matAtWork commented 8 years ago

Just published v2.6.10 which (I hope) fixes this issue. Please check on the link above.

I've not updated fast-async (yet) as I'll do that when I'm ready to ship v3. You should get the update automatically if you uninstall and re-install fast-async in your project.

Let me know how it goes, and thanks for the report

Rush commented 8 years ago

It seems to have fixed it. Also, sourcemaps seem to work but they are not enabled by default. Is it a bug?

I had to add the option to .babelrc

{
  "presets": [],
  "plugins": [["fast-async", {
    "compiler": {
      "sourcemap": true
    } 
  }]]
}
Error
    at $ForStatement_1_loop (/code/fast-async-issue-13/test.js:4:17)
    at Function.boundThen [as then] (/code/fast-async-issue-13/test.js:122:21)
    at /code/fast-async-issue-13/test.js:1:1
    at boundThen (/code/fast-async-issue-13/test.js:122:21)
    at liveBackup (/code/fast-async-issue-13/test.js:1:1)
    at Object.<anonymous> (/code/fast-async-issue-13/test.js:13:1)
    at Module._compile (module.js:556:32)
    at loader (/code/fast-async-issue-13/node_modules/babel-register/lib/node.js:143:5)
    at Object.require.extensions.(anonymous function) [as .js] (/code/fast-async-issue-13/node_modules/babel-register/lib/node.js:153:7)
    at Module.load (module.js:473:32)
Error
    at $ForStatement_1_loop (/code/fast-async-issue-13/test.js:4:17)
    at $ForStatement_1_next (/code/fast-async-issue-13/test.js:1:1)
    at /code/fast-async-issue-13/test.js:5:5
    at then (/code/fast-async-issue-13/test.js:115:126)
    at process._tickCallback (internal/process/next_tick.js:103:7)
    at Module.runMain (module.js:592:11)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3
Error
    at $ForStatement_1_loop (/code/fast-async-issue-13/test.js:4:17)
    at $ForStatement_1_next (/code/fast-async-issue-13/test.js:1:1)
    at /code/fast-async-issue-13/test.js:5:5
    at then (/code/fast-async-issue-13/test.js:115:126)
    at process._tickCallback (internal/process/next_tick.js:103:7)
    at Module.runMain (module.js:592:11)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3
foo
async function liveBackup() {
  let files = ['aaa', 'fff', 'ewrrewewrewr'];
  for(let file of files) {
    console.log(new Error().stack);
    await Promise.resolve();
  }
  for(let file of files) {
    await Promise.resolve();
  }
  return 'foo';
}

liveBackup().then(console.log);
Rush commented 8 years ago

This is really weird. After running my test after couple minutes, the sourcemaps stopped working and then on another run they worked fine (in my small test, anyway). There is something really quirky going on.

Anyway, the performance of your project is impressive but without sourcemaps this cannot be used in production. :-(

Rush commented 8 years ago

Anyway, I'm gonna close this issue and open another one.

jscinoz commented 6 years ago

This issue is still present in 2018 as of fast-async 6.3.0.

matAtWork commented 6 years ago

Can you be more specific? The example given seems to transform and work fine:

async function liveBackup(files) {
  for(let file of files) {
    log(await Promise.resolve(file));
  }
  for(let file of files) {
    log(await Promise.resolve(file));
  }
}

liveBackup([654,"abc",34]) ;

http://nodent.mailed.me.uk/#async%20function%20liveBackup(files)%20%7B%0A%20%20for(let%20file%20of%20files)%20%7B%0A%20%20%20%20log(await%20Promise.resolve(file))%3B%0A%20%20%7D%0A%20%20for(let%20file%20of%20files)%20%7B%0A%20%20%20%20log(await%20Promise.resolve(file))%3B%0A%20%20%7D%0A%7D%0A%0AliveBackup(%5B654%2C%22abc%22%2C34%5D)%20%3B~options~%7B%22mode%22%3A%22promises%22%2C%22promiseType%22%3A%22Zousan%22%2C%22noRuntime%22%3Afalse%2C%22es6target%22%3Afalse%2C%22wrapAwait%22%3Afalse%2C%22spec%22%3Afalse%7D

jscinoz commented 6 years ago

It seems my issue is a bit different (same error, but to do with a name collision across distinct branches of an if statement where const is used within - perfectly valid and works correctly when fast-async isn't involved.

I suspect it's an interaction with another Babel plugin in our project - I'll comment back here when I've figured it out.

jscinoz commented 6 years ago

Seems it was an issue with the ordering of Babel plugins in our project - having fast-async last fixed it.