Skip to content

[New] add jsx-no-useless-fragment rule #2261

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

Merged
merged 1 commit into from
Sep 8, 2019

Conversation

golopot
Copy link
Contributor

@golopot golopot commented May 5, 2019

fixes #2042

This rule disallows fragments with less than 2 children.

@ljharb
Copy link
Member

ljharb commented May 5, 2019

Should this rule be autofixable?

@golopot
Copy link
Contributor Author

golopot commented May 6, 2019

Autofixing <>foo</>, <>{foo}</> might break people's code if they use PropTypes.element, or uses some type checker.

Case like <><Foo /></> looks ok to be autofixable.

@ljharb
Copy link
Member

ljharb commented May 13, 2019

hm, maybe if there's cases that aren't safe to autofix, those should be separately controllable by an option?

@golopot golopot force-pushed the jsx-no-useless-fragment branch from 017c476 to 17cc2c7 Compare May 16, 2019 13:29
@golopot
Copy link
Contributor Author

golopot commented May 16, 2019

Now fragments in html element are reported.

Example:

<section>
  <>
    <div />
    <div />
  </>
</section>

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So any time a fragment is being passed to an HTML element, it should be autofixable to inline the fragment's children, no?

@golopot
Copy link
Contributor Author

golopot commented May 18, 2019

Now a fragment is fixable if it is not an outermost element, except for when it might add extra spaces. The fix is a simple removal of openingElement and closingElement.

// fixing by simply removing fragments will add a space between "git" and "hub"
<div>
  git<>
    hub
  </>
</div>

@ljharb
Copy link
Member

ljharb commented May 18, 2019

Couldn't that be fixed into:

<div>
  git{`
    hub
  `}
</div>

or

<div>
  git{' hub '}
</div>

?

@golopot
Copy link
Contributor Author

golopot commented May 18, 2019

I think it is ok to leave these cases without a fix. Your fixed code both render "git hub", but my code renders "github".

@ljharb
Copy link
Member

ljharb commented May 18, 2019

ah, git{'hub'} or just github then? but sure, leaving it without a fix is fine too.

@captbaritone
Copy link
Contributor

Hey! I'm curious what the status of this PR is. I just wrote a similar rule myself before I saw this PR.

If you plan on coming back and getting this across the finish line, I'll just add one more thing to consider: Fragments can be given props. Specifically key.

If you don't plan on coming back to this, I'd be happy to open a PR with the one I've written once we having it running for a little while and I'm confident there are no major issues.

@ljharb
Copy link
Member

ljharb commented Jul 11, 2019

@captbaritone please don't open a second PR; it'd be better to update this one.

@golopot golopot force-pushed the jsx-no-useless-fragment branch from 7425244 to ce4336f Compare July 12, 2019 11:49
@@ -0,0 +1,50 @@
# Disallow unnecessary fragments (react/jsx-no-useless-fragment)

A fragment is redundant if it contains only one child, or if it is the child of a html element.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we need to also add prose and examples and tests showing that a fragment is not useless if it has a key prop and also appears in a context where a key prop makes sense (ie, inside an array literal or returned from a function)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a version just let pass any keyed fragment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a fragment has only a single JSXElement child, it is still useless even if it does have a key.

For example:

<React.Fragment key={someVar}>
  <div />
</React.Fragment>

Should be refactored/fixed to:

<div key={someVar} />

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not entirely true; something might be cloning the element.

@captbaritone
Copy link
Contributor

Another case which is worth considering:

<>
  In a case like this, fragments offer a nice way to split long
  strings across multiple lines. Using a fragment here, while
  technically useless, is probably nicer than using a quoted
  string.
</>

@ljharb
Copy link
Member

ljharb commented Aug 31, 2019

@captbaritone given that you can use backticks for the same effect, it seems like using a fragment there is strictly worse than the alternative (making an object instead of a string literal)

@captbaritone
Copy link
Contributor

@ljharb from a technical/performance perspective, I think you’re right, but from a readability/developer experience, perspective I think there’s a reasonable argument for fragments. With back ticks, new lines and indentation can be annoying write cleanly. For example, I don’t think Prettier will word-wrap strings in back ticks for you (since it can’t know if it’s safe) where as I believe it will word wrap JSX for you since it knows line breaks are safe in white space. (I’m on mobile so I haven’t actually checked)

So, forcing back ticks may also force users to manually handle line wrapping, which can be a bit annoying.

@golopot
Copy link
Contributor Author

golopot commented Sep 1, 2019

Using fragments for linebreak-less strings can help avoid max-len. Also makes the string formattable by prettier.

<Author
  name="eeee"
  description="eee  eeeeee eee eeeee. eeeeeeee eeee  eeeeeeeeeeeee eeee. eeeeeee eee eeeeeeeeee ee eee eeeeeee eee."
/>;
<Author
  name="eeee"
  description={
    <>
      eee eeeeee eee eeeee. eeeeeeee eeee eeeeeeeeeeeee eeee. eeeeeee eee
      eeeeeeeeee ee eee eeeeeee eee.
    </>
  }
/>;

@ljharb
Copy link
Member

ljharb commented Sep 1, 2019

max-len should just be wholesale disabled anyways :-) but ok, fair

@golopot golopot force-pushed the jsx-no-useless-fragment branch from 05a5abb to c025438 Compare September 1, 2019 10:01
@golopot
Copy link
Contributor Author

golopot commented Sep 1, 2019

I have rewrite the fixer to fixes more cases.

@golopot golopot force-pushed the jsx-no-useless-fragment branch from 973b027 to 177a4c5 Compare September 7, 2019 17:51
@ljharb ljharb force-pushed the jsx-no-useless-fragment branch from 177a4c5 to 7ccff10 Compare September 8, 2019 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Rule proposal: detect unnecessary use of fragments
3 participants