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

ts-migrate is inserting comments in JSX components #150

Open
flupke opened this issue Nov 3, 2021 · 16 comments
Open

ts-migrate is inserting comments in JSX components #150

flupke opened this issue Nov 3, 2021 · 16 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@flupke
Copy link

flupke commented Nov 3, 2021

I'm giving ts-migrate a try to migrate a large gatsby project. My issue is JS comments are inserted in components:

export function Input(props: any) {
  return props.readonly ? (
    <div className={style.readonlyInputField} style={props.style}>
      {props.value}
    </div>
  ) : (
    <div className={style.inputField} style={props.style}>
      // @ts-expect-error ts-migrate(2607) FIXME: JSX element class does not support attributes beca... Remove this comment to see the full error message
      <RawInput className={style.input} {...props} />
    </div>
  )
}

This makes them visible in the rendered output. They should be inserted with braces instead:

export function Input(props: any) {
  return props.readonly ? (
    <div className={style.readonlyInputField} style={props.style}>
      {props.value}
    </div>
  ) : (
    <div className={style.inputField} style={props.style}>
      {/* @ts-expect-error ts-migrate(2607) FIXME: JSX element class does not support attributes beca... Remove this comment to see the full error message */}
      <RawInput className={style.input} {...props} />
    </div>
  )
}

I ran npx -p ts-migrate -c "ts-migrate-full ." at the root of the project. During the migration I saw a lot of occurrences of this error:

Error: [react-props][src/components/multiStates.tsx] Error:
 TypeError: Cannot read property 'length' of undefined
    at isReactSfcFunctionDeclaration (/home/flupke/src/jitter/jitter.video/frontend/node_modules/ts-migrate-plugins/build/src/plugins/utils/react.js:32:40)
    at Array.filter (<anonymous>)
    at Object.getNumComponentsInSourceFile (/home/flupke/src/jitter/jitter.video/frontend/node_modules/ts-migrate-plugins/build/src/plugins/utils/react.js:76:10)
    at Object.createPropsTypeNameGetter (/home/flupke/src/jitter/jitter.video/frontend/node_modules/ts-migrate-plugins/build/src/plugins/utils/react-props.js:239:41)
    at Object.run (/home/flupke/src/jitter/jitter.video/frontend/node_modules/ts-migrate-plugins/build/src/plugins/react-props.js:42:48)
    at Object.migrate (/home/flupke/src/jitter/jitter.video/frontend/node_modules/ts-migrate-server/build/src/migrate/index.js:65:46)
    at async Object.handler (/home/flupke/src/jitter/jitter.video/frontend/node_modules/ts-migrate/build/cli.js:130:22)

I have 108 .tsx files in the project so this will be cumbersome to fix, did I do something wrong during the conversion?

@flupke
Copy link
Author

flupke commented Nov 4, 2021

Looking at ts-migrate code, the error comes from here, in ts-migrate-plugins/build/src/plugins/utils/react.js:

function isReactSfcFunctionDeclaration(functionDeclaration) {
    return (functionDeclaration.name != null &&
        /^[A-Z]\w*$/.test(functionDeclaration.name.text) &&
        functionDeclaration.parameters.length <= 2);  // <--------------
}

I tried handling the functionDeclaration.parameters === undefined case and the other similar errors I had, but this gave the same result as in the original issue.

@lnrdgmz
Copy link

lnrdgmz commented Nov 6, 2021

I'm experiencing the same problem. I tried to pinpoint the problem by stepping through with the debugger, but I was unable to find the point of failure.

I changed versions of TypeScript to see if that could be the issue, and found the issue does not exist when using v4.2.4. With this version, I get the expected output after running yarn ts-migrate migrate:

export default ({
  name
}: any) => {
  return (
    // @ts-expect-error ts-migrate(7026) FIXME: JSX element implicitly has type 'any' because no i... Remove this comment to see the full error message
    <div>
      {/* @ts-expect-error ts-migrate(7026) FIXME: JSX element implicitly has type 'any' because no i... Remove this comment to see the full error message */}
      <div>Hello, {name}!</div>
    {/* @ts-expect-error ts-migrate(7026) FIXME: JSX element implicitly has type 'any' because no i... Remove this comment to see the full error message */}
    </div>
  )
}

With versions 4.3.2 and 4.4.4, I get the following:

export default ({
  name
}: any) => {
  return (
    // @ts-expect-error ts-migrate(7026) FIXME: JSX element implicitly has type 'any' because no i... Remove this comment to see the full error message
    <div>
      // @ts-expect-error ts-migrate(7026) FIXME: JSX element implicitly has type 'any' because no i... Remove this comment to see the full error message
      <div>Hello, {name}!</div>
    // @ts-expect-error ts-migrate(7026) FIXME: JSX element implicitly has type 'any' because no i... Remove this comment to see the full error message
    </div>
  )
}

@flupke
Copy link
Author

flupke commented Nov 9, 2021

Good detective work thank you! I'm sure this will help others a great deal, editing the hundreds of comments by hand was not fun at all :]

@sharmilajesupaul
Copy link

@Rudeg @brieb @lencioni +1 i'm also able to repro this bug in typescript@4.4.4
A meta question: can we add a help-wanted label? so that the community can pick those up and contribute these fixes?

@matanwerbner
Copy link

@Rudeg This is still reproducing on my project, I've attempted to change my project typescript version to 4.2.4, but it still happens. Any idea if #170 requires something extra to work?

@mirague
Copy link

mirague commented Aug 31, 2022

Also seeing exactly these issues: normal javascript // comments inserted inside TSX.

Tried rolling back to v0.1.28 which included TypeScript 4.2.4 but with no luck.

@Calidus
Copy link
Contributor

Calidus commented Nov 7, 2022

I am noticing that this only happens on the last comment of each file. I have a number jsx files that generate +20 @ex-expect-error comments but only last comment uses // instead of the curly brackets.

typescript 4.8.4

Edit:

Its seems to be adding // @ts-expect-error TS(7026): JSX element implicitly has type 'any' because no i... Remove this comment to see the full error message before the last closing tag.

@Calidus
Copy link
Contributor

Calidus commented Nov 7, 2022

I think there might be two related issues here:

  • why are we getting @ts-expect-error on closing tags? That just seems wrong to me if we have one on the opening tag.
  • ts-ignore doesn't properly handle adding comments to closing tags in tsx files. ts-ignore.inJsxText is returning false for closing tags.

Calidus added a commit to Calidus/ts-migrate that referenced this issue Nov 7, 2022
- inJsxText would always return false if the position it was given was
a closing tag. This caused ts-ignore to not properly comment
@ts-expect-error messages when they were added to tsx files.
- Issue airbnb#150
Calidus added a commit to Calidus/ts-migrate that referenced this issue Nov 7, 2022
- inJsxText would always return false if the position it was given was
a closing tag. This caused ts-ignore to not properly comment
@ts-expect-error messages when they were added to tsx files.
- Issue airbnb#150
@Calidus
Copy link
Contributor

Calidus commented Nov 8, 2022

With the release of 0.1.35 I am not seeing any more incorrectly formatted comments in TSX files for the large project I am working on. I am still getting what I think are superfluous ts-expect-errors but that probably needs to be address with a new issue.

@szamanr
Copy link

szamanr commented Nov 23, 2022

i am still experiencing this on 0.1.35. i'm using typescript 4.8.4 and react 18.2.0.

could it be platform-dependent? i'm on linux, and my colleague on mac doesn't experience this issue.

@Calidus
Copy link
Contributor

Calidus commented Nov 28, 2022

i am still experiencing this on 0.1.35. i'm using typescript 4.8.4 and react 18.2.0.

could it be platform-dependent? i'm on linux, and my colleague on mac doesn't experience this issue.

I was using Ubuntu via WSL with typscript 4.8.4. Are you seeing the miss formatted comments right before closing tags or in other locations?

@szamanr
Copy link

szamanr commented Nov 28, 2022

i am still experiencing this on 0.1.35. i'm using typescript 4.8.4 and react 18.2.0.
could it be platform-dependent? i'm on linux, and my colleague on mac doesn't experience this issue.

I was using Ubuntu via WSL with typscript 4.8.4. Are you seeing the miss formatted comments right before closing tags or in other locations?

here's a screenshot. it's the git diff view: grey = before running script, green = after running script.

ts-migrate broke

@Calidus
Copy link
Contributor

Calidus commented Nov 28, 2022

You may want to see verify the package lock file between you and your coworker to make sure you both using the exact same versions. I did a quick test of isJsxText using function foo { return ( HTML HERE ); } and it seems to be fine. I would recommend coming up with a minimal repo.

@noorulh06
Copy link

For me, It worked with v4.7.2 which is the same version defined in package.json of ts-migrate repo.

@jcurtis
Copy link

jcurtis commented Sep 25, 2023

This is broken with typescript 4.9.5, but not when I downgrade to 4.7.2.

@AlexaDeWit
Copy link

AlexaDeWit commented Feb 26, 2024

I'm getting comments inside the (t|j)sx expressions with TypeScript 5.0.4 Although I was able to get around this my temporarily downgrading to 4.7.2 and upgrading back, with only about ~10 manual edits needed after.

Thanks @jcurtis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests