Extra parens inserted around JSX after attributes are added

[michaldudak]

We're encountering an issue that prevents us from upgrading from 0.13.1 to 0.14.0 (mui/material-ui#34680).

It seems that when a codemod adds a JSX attribute, the whole JSX block in a return statement gets wrapped in extra parens (even if it had parens before).

I managed to create a minimal repro:

index.js

const fs = require("fs");
const j = require("jscodeshift");

const source = fs.readFileSync("source.js", "utf8");

function addAttribute(path) {
  const attributes = path.node.openingElement.attributes;

  attributes.push(
    j.jsxAttribute(j.jsxIdentifier("data-foo"), j.literal("bar"))
  );
}

const result = j(source).find(j.JSXElement).forEach(addAttribute).toSource();
console.log(result);

source.js

import * as React from "react";

export default function WrappedLink(props) {
  return (
    <div>
      <a {...props} />
    </div>
  );
}

expected output

import * as React from "react";

export default function WrappedLink(props) {
  return (
    <div data-foo="bar">
      <a {...props} data-foo="bar" />
    </div>
  );
}

actual output

import * as React from "react";

export default function WrappedLink(props) {
  return (
    (<div data-foo="bar">
      <a {...props} data-foo="bar" />
    </div>)
  );
}

[ElonVolo]

This is something that is happening at the recast level. I temporarily downgraded jscodeshift's version of recast to 0.20.4 and the extra parentheses went away.

Do the extra parentheses actually introduce bugs/regressions into migrated mui projects, or is it more of an aesthetics thing?

[ElonVolo]

Here's what I believe is the issue.

The JSX node itself is node.extra.parenthesized, and the printer always adds parens when returning a multiline JSX node as a return statement.

This might be one of those tricky recast bugs that crosses the border into philosophical debate on what the "correct" behavior might be and how exceptions to rules to exceptions to rules might have to be calculated. You might be better off adding a prettier rule that strips the extra parentheses and running code through prettier after the migration.

benjamn/recast#1068 (comment)

[michaldudak]

Do the extra parentheses actually introduce bugs/regressions into migrated mui projects, or is it more of an aesthetics thing?

It's impossible to say right now. We haven't released our codemods with updated jscodeshift, so we don't know what our users may run into. The way our tests fail makes me believe it's only a matter of aesthetics.

This might be one of those tricky recast bugs that crosses the border into philosophical debate on what the "correct" behavior might be and how exceptions to rules to exceptions to rules might have to be calculated. You might be better off adding a prettier rule that strips the extra parentheses and running code through prettier after the migration.

Thanks a lot for investigating it! I don't think we can use any formatter as a part of the codemod, as we don't know the code style settings of codebases the codemods will run on.

I temporarily downgraded jscodeshift's version of recast to 0.20.4 and the extra parentheses went away

Does jscodeshift 0.14.0 work with recast 0.20.4 without any code changes? Perhaps pinning the recast version using yarn resolutions could be the way to go for us.

[Daniel15]

You might be better off adding a prettier rule that strips the extra parentheses and running code through prettier after the migration.

FWIW this is generally the approach I take these days, across a few languages. Rather than ensuring all codemods output properly formatted code, I instead use a tool like Prettier to auto-format the resulting code. At Meta, we have auto-formatters for the most commonly-used languages, and for example our Hacklang codemods don't even try to properly format the resulting code as they just rely on hackfmt to format the code.

The issue with that is that not every codebase uses auto-formatting...

[michaelfaith]

This impacts us as well, in the same way. Unit tests doing input/output comparisons (similar to what MUI's codemod test suite does) are breaking after the update, and it yields files now that don't conform to prettier rules.

[phitattoo]

Phiphuket99

[michaelfaith]

There appears to be a PR in recast to address this benjamn/recast#1406

[ElonVolo]

Aye, there is. And it may get merged into recast's main branch in the next year or two.

[Ariqun]

I remove it by regexp:

const REG_RETURN = /return\s+\((\s+)\(<([^\s>]+)/g;

function removeCircleBracket(string) {
  const matches = string.matchAll(REG_RETURN);

  if (matches) {
    [...matches].forEach((match) => {
      const [_, spaces, componentName] = match;
      const sl = spaces.length;

      const REG_DOUBLE_TAG = `(return\\s+\\(\\s{${sl}})\\((<${componentName}.+?\\s{${sl}}<\\/${componentName}>)\\)(\\s+\\))`;
      const REG_SELF_CLOSING = `(return\\s+\\(\\${sl})\\((<${componentName}.+?\\${sl}\\/>)\\)(\\s+\\))`;
      const REG_FULL = REG_DOUBLE_TAG + '|' + REG_SELF_CLOSING;

      const regexp = new RegExp(REG_FULL, 'gs');

      string = string.replace(regexp, (match, g1, g2, g3, g4, g5, g6) => {
        const isDoubleCondition = g1 && g2 && g3;
        const isSelfCondition = g4 && g5 && g6;

        if (isDoubleCondition) {
          return g1 + g2 + g3;
        }

        if (isSelfCondition) {
          return g4 + g5 + g6;
        }

        return match;
      });
    });
  }

  return string;
}

removeCircleBracket(root.toSource(toSourceOptions));

Doesn't handle all cases, but most + it's necessary for the code to be written correctly.