MatAtBread / fast-async

605 stars 21 forks source link

broken interaction with babel modules transform #33

Closed rainbow-alex closed 7 years ago

rainbow-alex commented 7 years ago

To reproduce:

// foo.js
export default async function bar() {}
bar.SOMETHING = 3
// .babelrc
{
  "plugins": [
    "transform-es2015-modules-commonjs",
    "fast-async"
  ]
}

And run npm install fast-async babel-cli babel-plugin-transform-es2015-modules-commonjs.

Running ./node_modules/.bin/babel-node foo.js then gives:

bar.SOMETHING = 3;
^

ReferenceError: bar is not defined
matAtWork commented 7 years ago

This appears to be becuase fast-async treats the right of the export default as a FunctionExpression and not a FunctionDeclaration, meaning the identifier bar is only defined within the body of the function. It's a bug. I'll try to fix it as soon as I get a chances, but the workaround meanwhile is:

export default bar;
async function bar() {}
bar.SOMETHING = 3
matAtWork commented 7 years ago

Actually, this isn't the bug I thought it was. Using nodent (which does all the work under fast-async) from the CLI, all looks good:

foo.js

export default async function bar() {}
bar.SOMETHING = 3

Output without Babel (from the same dir the test you specified is in):

$ node_modules/nodent/nodent.js --out --use=promise foo.js
export default function bar() {
    return new Promise((function ($return, $error) {
        return $return();
    }).$asyncbind(this));
}

bar.SOMETHING = 3;

Output the AST, verifying foo is a FunctionDeclaration:

node_modules/nodent/nodent.js --minast --use=promise foo.js
{
  "type": "Program",
  "body": [
    {
      "type": "ExportDefaultDeclaration",
      "declaration": {
        "type": "FunctionDeclaration",
        "id": {
          "type": "Identifier",
          "name": "bar"
        },
        "generator": false,
        "expression": false,
        "params": [],
        "body": 
.... ....
      }
    }
  ],
  "sourceType": "script"
}

Run nodent and then babel from the CLI - works as expected (.babelrc only contains "transform-es2015-modules-commonjs")

$ node_modules/nodent/nodent.js --out --use=promise foo.js > foo_nodent.js
$ ./node_modules/babel-cli/bin/babel.js foo_nodent.js
"use strict";

Object.defineProperty(exports, "__esModule", {
    value: true
});
exports.default = bar;
function bar() {
    return new Promise(function ($return, $error) {
        return $return();
    }.$asyncbind(this));
}

bar.SOMETHING = 3;

Finally, running under the debugger, when babel passes the AST to nodent (within fast-async), it appears that Babel incorrectly parses the export async function bar(){} as it is (indeed) a FunctionExpression. Why this should be the case, I don't know, as testing babylon6 on astexplorer.net works as expected.

I'll take another look as I like a mystery, but I don't think this is a fast-async issue as it is passed an FunctionExpression and therefore returns one, with the consequentially limited scope of the function identifier.

matAtWork commented 7 years ago

Incidentally, running babel then nodent [EDIT] DOESN'T work - it generates function bar() as an expression on the right hand side of the exports.default:

$ ./node_modules/babel-cli/bin/babel.js foo.js | ./node_modules/nodent/nodent.js --out --use=promise --noruntime
"use strict";
Object.defineProperty(exports, "__esModule", {
    value: true
});
exports.default = function bar() {
    return new Promise((function ($return, $error) {
        return $return();
    }).bind(this));
};
bar.SOMETHING = 3;

This appears to be a babel parsing bug. Do you know if it works with the Babel async transformers?

matAtWork commented 7 years ago

Final test, with an empty .bablerc, it appears that something in babel is mangling the AST (no fast-async or nodent involved at all):

$ echo 'export default async function bar() {}' | ./node_modules/babel-cli/bin/babel.js
export default (async function bar() {});

$ echo 'export default function bar() {}' | ./node_modules/babel-cli/bin/babel.js
export default function bar() {}

The former is being parsed as a FunctionExpression, so babel wraps it in parentheses. The latter is not, so the parens are absent.

I'd report this as a (minor) issue in bable. The only possibility I have to fix it would be to force the node to be a FunctionDeclaration, but this would potentially expose the identifier out of it's intended scope, and would break on export default function(){}.

If you're satisfied with the explanation and workaround, please close the issue, or otherwise let me know if you have any info that might be useful.

matAtWork commented 7 years ago

I think this issue has already been identified in Babel here https://github.com/babel/babylon/issues/374, so I'll close this for now. It maybe that an earlier revision of Babel will work as expected.

Rush commented 7 years ago

@matAtWork unless you want to treat Issues as a TODO list, issues shouldn't be closed solely because the problem lays in upstream.

Rather the opposite, issue should be kept open to:

In the end it's your call, just a friendly tip here.

matAtWork commented 7 years ago

The issue remains on github so users can search for it, but since there is no possible resolution, I close them. I take you point, but this is one-man project and whilst I try to investigate and fix any issues, I don't have the resources to maintain and groom a backlog. May I should tag it "won't fix" rather than close it.

If you wish to add it to the README (that in babylon 6.16.1 the construct export async function x(){} is parsed incorrectly), I'd happily accept a PR.

rainbow-alex commented 7 years ago

@matAtWork Thanks for the quick and extensive follow-up. I'll file the bug with babel and hope for a fix :)

matAtWork commented 7 years ago

@rainbow-alex - NP. Incidentally, I posted updated the Babylon issue, and they're already aware of it.