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

jsx-curly-brace-presence warns incorrectly on trailing whitespace #2427

Closed
taion opened this issue Oct 1, 2019 · 13 comments · Fixed by #2431
Closed

jsx-curly-brace-presence warns incorrectly on trailing whitespace #2427

taion opened this issue Oct 1, 2019 · 13 comments · Fixed by #2431

Comments

@taion
Copy link
Contributor

taion commented Oct 1, 2019

This is a regression from #2409, cc @vedadeepta @ljharb

Per #2409 (comment), when running autofix with this change, I now get e.g.

           <div>
-            Confirm{' '}
+            Confirm
             <button type="button" onClick={this.onClickYes}>

The rule may be correct in isolation, but this interacts very poorly with the standard automatic removal of trailing whitespace.

@vedadeepta
Copy link
Contributor

@taion you want to keep the traling white space iff it is inside curly braces ?

I think the rule can be adjusted for this special case but i'm not sure if markup should be written like this though.

@taion
Copy link
Contributor Author

taion commented Oct 1, 2019

jsx-curly-brace-presence should not warn when the curly braces surround trailing whitespace.

How else would you write this markup? In general it's not that odd to put a line break in this sort of place. It's e.g. what Prettier does.

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

That also makes total sense.

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

@vedadeepta indeed, putting whitespace inside curlies is the only way to write it so that HTML doesn't collapse it.

@vedadeepta
Copy link
Contributor

@taion makes sense to account for your case, earlier i was thinking more in terms of css paddings/margins for creating spaces.

@ryansully
Copy link

ryansully commented Oct 2, 2019

I think I'm experiencing a related situation where I need whitespace between children, but the whitespace child warns incorrectly.

<button>
  <i className="fa fa-check" />
  {' '}
  Confirm
</button>

In this case I'm also using jsx-one-expression-per-line, so it's not just an issue of trailing whitespace, but of any use of whitespace children.

There is also the case if there needs to be non-whitespace text with leading or trailing whitespace, or both:

<p>
  {'The '}
  <strong>rest</strong>
  {' of the '}
  <a href="/">site</a>
  {' already.'}
</p>

An autofix of this results in joined words with only one space in the middle.

@taion
Copy link
Contributor Author

taion commented Oct 4, 2019

This still led to a slightly unsafe autofix in

--- a/www/src/examples/Navbar/Brand.js
+++ b/www/src/examples/Navbar/Brand.js
@@ -28,7 +28,7 @@
         height="30"
         className="d-inline-block align-top"
       />
-      {' React Bootstrap'}
+      React Bootstrap
     </Navbar.Brand>
   </Navbar>
 </>;

But I think I'm okay with moving all that whitespace to trailing.

Thanks!

@taion
Copy link
Contributor Author

taion commented Oct 4, 2019

And also:

-            <React.Fragment key={idx}>
-              {' / '}
-              {crumb}
-            </React.Fragment>
+            <React.Fragment key={idx}>/{crumb}</React.Fragment>

So this is a rule in the sense that both this and the above are things that I'd want to fix.

But in both cases the autofix ate my whitespace, which is not ideal.

@ljharb
Copy link
Member

ljharb commented Oct 4, 2019

Both i consider bugs; could you file a new issue, or a PR with the test cases?

@SirCameron
Copy link

Has a new issue been filed? Why not reopen this issue?

@ljharb
Copy link
Member

ljharb commented Oct 9, 2019

It’s cleaner to avoid an issue being “closed” by multiple PRs, and it helps ensure that further comments don’t get lost by being in a closed issue.

@taion
Copy link
Contributor Author

taion commented Oct 9, 2019

Sorry, I've been meaning to put together at least the failing test, but I've been jammed for time. Let me open a new issue at least.

@taion
Copy link
Contributor Author

taion commented Oct 9, 2019

ref #2454

stefan-wullems pushed a commit to stefan-wullems/eslint-plugin-react that referenced this issue Nov 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants