[Bug]: Edge case in TSX: <T,>(t: T) => t

[hyrious]

💻

• Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

const { transformSync } = require('@babel/core')

let result = transformSync(`
  const fn = <T,>(t: T) => t;
`, {
  babelrc: false,
  configFile: false,
  parserOpts: {
    plugins: [
      'jsx',
      'typescript',
    ]
  },
})

console.log(result.code)

Configuration file name

No response

Configuration

No response

Current and expected behavior

The current output:

const fn = <T>(t: T) => t;

This is valid in TS, but invalid in TSX, because <T> is treated as a tag in TSX context.

Expected output:

const fn = <T,>(t: T) => t;

Environment

  System:
    OS: macOS 11.5.2
  Binaries:
    Node: 16.9.1 - /usr/local/bin/node
    Yarn: 1.22.11 - /usr/local/bin/yarn
    npm: 7.23.0 - /usr/local/bin/npm

Possible solution

No response

Additional context

Original Issue: vitejs/vite#4949.

[babel-bot]

Hey @hyrious! 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."

[hyrious]

Possible solution: keep trailing comma in type list:

// in babel-parser/src/plugins/typescript/index.js tsParseDelimitedListWorker:
let comma = false;
// ...for (;;) {...
  if (this.tsIsListTerminator(kind)) {
    result.hasTrailingComma = comma;
    break;
  }
  if (this.eat(tt.comma)) {
    comma = true;
    continue;
  }

// in babel-generator/src/generators/typescript.ts TSTypeParameterInstantiation:
if (node.params.hasTrailingComma) this.token(",");
this.token(">");

[nicolo-ribaudo]

We can just always unconditionally print a trailing comma.

[nicolo-ribaudo]

@hyrious Since you already looked at the @babel/generator code, would you like to open a PR? Otherwise I'll mark this as a [good first issue] for anyone who is interested in contributing.

---

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. Write a comment there to let other possible contributors know that you are working on this bug.
2. Fork the repo
3. Run git clone https://github.com/<YOUR_USERNAME>/babel.git && cd babel
4. Run yarn && make bootstrap
5. Wait ⏳
6. Run make watch (or make build whenever you change a file)
7. Add a test (only input.js; output.js will be automatically generated)
8. Update the code!
9. yarn jest 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 generator and they will be automatically updated.
10. If it is working, run make test to run all the tests
11. Run git push and open a PR!

[hyrious]

@nicolo-ribaudo Thank you for inviting me. However I'll not be able to use *nix system in the next few days.
So I guess you could do the latter.

The "solution" I posted above was just a quick & dirty hack into node_modules, it just passed this single edge case and didn't follow common practices (it stores a value in an array). It's difficult for me to refactor it into a more robust one in such large code base.

Always printing trailing comma in type list seems promising but may break many other things. I don't know it very much. So I'll leave this chance to other experienced programmers.

[ChaitanyaGadodia]

Hi! Can I pick this bug?

[fedeci]

It's yours!

[ozanhonamlioglu]

@nicolo-ribaudo

If this issue is not maintained since September, can I take it? Thanks.

[nicolo-ribaudo]

Yes 👍

[ozanhonamlioglu]

Hi @nicolo-ribaudo, I have a question

I am planning to put my test input into babel-generator/test/fixtures/typescript/arrow-function-generic-tsx
But looks like there is an error, because when I run the yarn jest generator it doesn't check the any input.js files inside fixtures folder and it doesn't generate output.js files. What am i doing wrong?

Also any hint that you want to support me is nice for me. Thanks

[nicolo-ribaudo]

Do you have a branch or draft PR I could look at to see why it's not working?

[ozanhonamlioglu]

In that screenshot I actually didn't changed or added any files, I just run the jest generator directly, so i can't give you valuable branch name.
Additionally here is my fork, it has the same hash.

See, nothing is changed.
If you can correct that currently it works on your machine, I can remove and install it again. Maybe something caused issue on my mac.

[ozanhonamlioglu]

Do you have a branch or draft PR I could look at to see why it's not working?

@nicolo-ribaudo I think there is a problem.

When I run yarn jest generator it wasn't running any of fixture tests. and also it was throwing this.

Then I removed every entry that has a test with t.stringLiteral inside babel-generator/test/index.js

After I run yarn jest generator now it started to generating output.js files inside fixtures; But now there are other errors.

I am thinking that babel-generator/test/index.js has an error and because of that this is happening.

[nicolo-ribaudo]

Did you run make bootstrap before running the tests? Are you working on an outdated fork/branch? 🤔

[ozanhonamlioglu]

This is my head hash ddd93b9 which is the current.

And yes I exactly did the steps you specified in here

> 2. Fork the repo
> 3. Run `git clone https://github.com/<YOUR_USERNAME>/babel.git && cd babel`
> 4. Run `yarn && make bootstrap`
> 5. Wait ⏳
> 6. Run `make watch` (or `make build` whenever you change a file)
> 7. Add a test (only `input.js`; `output.js` will be automatically generated)
> 8. Update the code!
> 9. `yarn jest 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 generator` and they will be automatically updated.
> 10. If it is working, run `make test` to run all the tests
> 11. Run `git push` and open a PR!

I am actually able to generate output.js files but the only way to generate them is to remove everything inside babel-generator/test/index.js except line 816 suites.forEach(function (testSuite) {...

Are you able to run yarn jest generator without any error with the current fork/branch?

[nicolo-ribaudo]

Yes, yarn jest generator works without problems on my machine 🤔

[ozanhonamlioglu]

Yes, yarn jest generator works without problems on my machine 🤔

Okay, never mind then, probably it is related to my env.
I actually started to dig in already, and it is going well and will update you soon I hope.
Thanks

[ozanhonamlioglu]

Hey @nicolo-ribaudo
I just opened a PR for the issue, and I want to ask one thing. I added a new test code inside an existing input file, because the naming of the parent folder was so right to me. Is that okay? or shall I create another folder and insert my code into it?

Thanks

[ozanhonamlioglu]

Hey @nicolo-ribaudo I just opened a PR for the issue, and I want to ask one thing. I added a new test code inside an existing input file, because the naming of the parent folder was so right to me. Is that okay? or shall I create another folder and insert my code into it?

Thanks

Anyway, I just separated my test codes, after a while reviewing test folders, it seems using separated folder for this case is fine.