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

Parenthesis on return statement has been removed #705

Closed
bigggge opened this issue Apr 16, 2020 · 16 comments · Fixed by #712
Closed

Parenthesis on return statement has been removed #705

bigggge opened this issue Apr 16, 2020 · 16 comments · Fixed by #712

Comments

@bigggge
Copy link

bigggge commented Apr 16, 2020

Reproduce example using jscodeshift:

https://astexplorer.net/#/gist/177972c375c147cd128f46354af81057/ae5a5014fa96340436dd3c68ff5063767dea8f4a

A simple demo without JSX
https://astexplorer.net/#/gist/ee27a86037e45714a0e4acaa57eca5c2/1a769021f57b29d21b3de0d8b59b44236decbcff

class Demo extends PureComponent {
   render() {
    return (
      <>
        <View
          style={{ backgroundColor: color.black }}
        />
        <View />
      </>
    );
  }
}

export default Demo;

to

class Demo extends PureComponent {
   render() {
    return <>
      <View
        style={{ backgroundColor: color.white }}
      />
      <View />
    </>;
  }
}

export default Demo;

I just want to change color.black to color.white, but parens has been removed.

@bigggge bigggge changed the title Parenthesis on JSX return statement has been removed Parenthesis on return statement has been removed Apr 16, 2020
@conartist6
Copy link
Contributor

I think this is because the printer doesn't understand expressionNode.extra.parenthesized, which is how babylon keeps track of parens.

@conartist6
Copy link
Contributor

I was incorrect about that being the cause.

@conartist6
Copy link
Contributor

Sorry, no, I was correct. But recast is correct that you don't need parens here. It just isn't respecting the fact that the source had parens, which it should.

The reason it doesn't understand that there were parens is as I stated. If you're able to pass the createParenthesizedExpressions option to babel this should work for you.

I'm also going to put up a PR that fixes this outright.

@conartist6
Copy link
Contributor

@bigggge could you verify that my changes in #712 fix your issue?

@conartist6
Copy link
Contributor

Actually I don't know quite how that will be possible. But since jscodeshift uses @babel/parser with recast, I believe I've essentially added a simplified version of the repro to the test suite.

conartist6 added a commit to conartist6/recast that referenced this issue May 25, 2020
conartist6 added a commit to conartist6/recast that referenced this issue May 25, 2020
conartist6 added a commit to conartist6/recast that referenced this issue May 26, 2020
@bigggge
Copy link
Author

bigggge commented May 26, 2020

Sorry, no, I was correct. But recast is correct that you don't need parens here. It just isn't respecting the fact that the source had parens, which it should.

The reason it doesn't understand that there were parens is as I stated. If you're able to pass the createParenthesizedExpressions option to babel this should work for you.

I'm also going to put up a PR that fixes this outright.

createParenthesizedExpressions option worked for me !

@bigggge
Copy link
Author

bigggge commented May 26, 2020

@bigggge could you verify that my changes in #712 fix your issue?

But these changes doesn't seem to work

@bigggge
Copy link
Author

bigggge commented May 26, 2020

And I test it using jscodeshift

@conartist6
Copy link
Contributor

When you say they don't work, what do you mean? How did you test them? If you can provide me enough info to reproduce a failure you're seeing I'll fix it.

@conartist6
Copy link
Contributor

@bigggge I've released @conartist6/recast@0.19.1-parens-fixes.0 with the changes from my PR since I need them while I wait for review. This should be the easiest way to test the changes in your project as well.

@bigggge
Copy link
Author

bigggge commented May 27, 2020

@conartist6 I push my test project and you can check it.
https://github.com/bigggge/recast-conartist6/blob/master/package.json#L7

@conartist6
Copy link
Contributor

@bigggge is there something for me to do? I ran the code and it seems to indicate that it is passing, but you'd have known that without me...

@bigggge
Copy link
Author

bigggge commented May 29, 2020

When you run npm test, you can see parens have been removed , like https://github.com/bigggge/recast-conartist6/blob/master/demo.js#L6-L9

@conartist6
Copy link
Contributor

@bigggge I tested again and it appears to be working perfectly. What did happen is that when my IDE has format on save turned on prettier stripped the extra parens when I saved the file. Once I stopped that happening there were no more problems.

no-bug

@conartist6
Copy link
Contributor

Oh interesting, but it does seem to lose the parens around return when your codeshift transform actually makes a change

@conartist6
Copy link
Contributor

I'm still not entirely sure what I'm seeing. I made a version of this test case in the recast test suite to cut out jscodeshift involvement and it works fine -- no parens are lost. I tried to replicate that result with jscodeshift by using the exact same babel parser (--parser babylon). Interestingly this helped, but didn't completely eliminate your problem.

function a() {
  let a = ( 1 + color.black );
  return (a + color.black);
}
// became
function a() {
  let a = ( 1 + color.white );
  return a + color.white;
}

So this is pretty odd. It stopped throwing away the parens on the first line, but still lost the ones around the return expression. I've got to head to work now, but I'd say this means two things: you should use --parser=babylon if you can in your real use case, and jscodeshift itself is involved somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants