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

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

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.

Co-Authored-By: Jordan Harband <ljharb@gmail.com>
lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
@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
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?

lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
@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.

@@ -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.

lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
@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)

tests/lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-useless-fragment.js Show resolved Hide resolved
@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
Copy link
Contributor Author

golopot commented Sep 1, 2019

I have rewrite the fixer to fixes more cases.

lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-useless-fragment.js Show resolved Hide resolved
lib/rules/jsx-no-useless-fragment.js Show resolved Hide resolved
tests/lib/rules/jsx-no-useless-fragment.js Outdated Show resolved Hide resolved
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