Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unary async arrow misprinted as concise async arrow when the binding identifier has leading comments #12500

Closed
1 task
MrEfrem opened this issue Dec 14, 2020 · 10 comments · Fixed by #13136
Closed
1 task
Assignees
Labels
i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator

Comments

@MrEfrem
Copy link

MrEfrem commented Dec 14, 2020

Bug Report

  • I would like to work on a fix!

Current behavior

Input Code

await knex.transaction(async (/** @type {any} */ trx) => {
});

is compiled to:

"use strict";

await knex.transaction(async
/** @type {any} */
trx => {});

and Node.JS throws so error:

SyntaxError: missing ) after argument list at Module._compile (internal/modules/cjs/loader.js:723:23) at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10) at Module.load (internal/modules/cjs/loader.js:653:32) at tryModuleLoad (internal/modules/cjs/loader.js:593:12) at Function.Module._load (internal/modules/cjs/loader.js:585:3) at Module.require (internal/modules/cjs/loader.js:692:17) at require (internal/modules/cjs/helpers.js:25:18) at Object.<anonymous> (/workspace/src/index.js:30:38) at Module._compile (internal/modules/cjs/loader.js:778:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)

Expected behavior

My source code should be compiled to this one without breaking async (/** comments */ param) => ...:

"use strict";

await knex.transaction(async (/** @type {any} */ trx) => {});

Babel Configuration (babel.config.js, .babelrc, package.json#babel, cli command, .eslintrc)

  • Filename: babel.config.js
{
  "presets": [
    [
      "@babel/env",
      {
        "targets": {
          "node": 10
        },
        "useBuiltIns": "usage",
        "corejs": 3
      }
    ]
  ]
}

Environment


  • Babel version(s): v7.12.10
  • Node/npm version: Node 10/yarn 1.22.5
  • OS: macOS 11.0.1
  • Monorepo: no
  • How you are using Babel: @babel/cli
  • Typescript: 4.1.3

Possible Solution

Additional context

I use Typescript JSDoc comments.

@babel-bot
Copy link
Collaborator

Hey @MrEfrem! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@nicolo-ribaudo
Copy link
Member

The problem is that Babel doesn't support compiling top-level await to CommonJS or to AMD. You can either compile to SystemJS modules, or use a bundler that supports it. (Bundlers have access to the whole module graph, so they can more easily transform top-level await).

@MrEfrem
Copy link
Author

MrEfrem commented Dec 14, 2020

It isn't the top level await. Above that part of source code I have:

/**
 * @param {{
    req: import('express').Request,
    res: import('express').Response,
    knex: any,
  }} options
 */
const someRoute = async ({ req, res, knex }) => {
  ...
  await knex.transaction(async (/** @type {any} */ trx) => {
    ...
  });
  ...
}
export default someRoute;

And it's the server side source code. I won't use any bundlers for it. I need to keep the original file tree.

My workaround is:

await knex.transaction(
      /** @type {(trx: any) => Promise<void>} */ async (trx) => {
}

then compiler doesn't break async (param) =>. But I thought that is bug in the compiler, no?

@MrEfrem
Copy link
Author

MrEfrem commented Dec 14, 2020

Changed REPL. And check REPL please. There isn't compiling to CommonJS.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 14, 2020

Thanks, this is indeed a bug with @babel/generator:
(just removing unrelated parts from your code)

async (/** @type {any} */ trx) => {
}

// ->

async
/** @type {any} */
trx => {};

@MrEfrem
Copy link
Author

MrEfrem commented Dec 14, 2020

No problems.

@JLHwung JLHwung changed the title Compile to wrong source code Unary async arrow misprinted as concise async arrow when the binding identifier has leading comments Dec 15, 2020
@nwalters512
Copy link
Contributor

Just ran into this myself in the context of introducing Jest to a project - was finally able to trace this back to Babel. If anyone can point me in the right direction, I'd like to try to contribute a fix for this.

@nicolo-ribaudo
Copy link
Member

Thanks!

The problem is likely in

if (
node.params.length === 1 &&
t.isIdentifier(firstParam) &&
!hasTypes(node, firstParam)
) {
if (
(this.format.retainLines || node.async) &&
node.loc &&
node.body.loc &&
node.loc.start.line < node.body.loc.start.line
) {
this.token("(");
if (firstParam.loc && firstParam.loc.start.line > node.loc.start.line) {
this.indent();
this.print(firstParam, node);
this.dedent();
this._catchUp("start", node.body.loc);
} else {
this.print(firstParam, node);
}
this.token(")");
} else {
this.print(firstParam, node);
}
} else {
this._params(node);
}
, where we decide how to print the arrow function parameters.

If it helps, you can reproduce it with this code:

const input = `
fn(async (/* comment */ x) => {})
`;

const ast = parser.parse(input);

console.log(generator.default(ast));

If it is the first time that you contribute to Babel, follow these steps: (you need to have make and yarn available on your machine)

  1. Fork the repo
  2. Run git clone https://github.com/<YOUR_USERNAME>/babel.git && cd babel
  3. Run yarn && make bootstrap
  4. Wait ⏳
  5. Run make watch (or make build whenever you change a file)
  6. Add a test in packages/babel-generator/test/fixtures/comments (only input.js; output.js will be automatically generated)
  7. Update the code!
  8. yarn jest babel-generator to run the tests
    • If some test outputs don't match but the new results are correct, you can delete the bad output.js files and run the tests again
    • If you prefer, you can run OVERWRITE=true yarn jest babel-generator and they will be automatically updated.
  9. If it is working, run make test to run all the tests
  10. Run git push and open a PR!

@nwalters512
Copy link
Contributor

Thanks for the pointers! Excited to make my first contribution to Babel - I opened up a PR at #13136.

Re: yarn && make bootstrap, it looks like make bootstrap already runs yarn install - it could save a small amount of time for future recipients of these instructions to just have folks run make bootstrap 🙂

@nicolo-ribaudo
Copy link
Member

Oh thanks; I never updated those instructions since I wrote them the first time 😅

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 13, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants