Skip to content

jsx-newline: support for checking newline at the start and end of children#3678

Draft
burtek wants to merge 1 commit intojsx-eslint:masterfrom
burtek:jsx-newline
Draft

jsx-newline: support for checking newline at the start and end of children#3678
burtek wants to merge 1 commit intojsx-eslint:masterfrom
burtek:jsx-newline

Conversation

@burtek
Copy link
Copy Markdown
Contributor

@burtek burtek commented Jan 11, 2024

Fixes #3663

Comment on lines +49 to +52
checkStartEnd: {
default: false,
type: 'boolean',
},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ljharb this makes sense but I wonder if we shouldn't also include option for banning start/end newlines while forcing empty lines between children 🤔

So this might actually become an enum of force | prevent rather than boolean. What do you think, what should I go with?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or we might go with:

undefined and false = do nothing
true = behaves same as between children (as per prevent)
force = forces an empty line
prevent = prevents empty line

And it will still take allowMultilines into account

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 11, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.74%. Comparing base (3730edb) to head (1b3be40).
⚠️ Report is 205 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3678      +/-   ##
==========================================
+ Coverage   97.65%   97.74%   +0.08%     
==========================================
  Files         132      132              
  Lines        9397     9405       +8     
  Branches     3445     3446       +1     
==========================================
+ Hits         9177     9193      +16     
+ Misses        220      212       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Jan 12, 2024

prevent: true or false obv need to continue working.

I like the idea of effectively deprecating the boolean config, and instead allowing an enum of 'ignore' (false), 'siblings' (true), 'adjacent' (lexically adjacent, ie everything) - that way if someone wants different options we can easily add them later, but for now we're only adding adjacent to cover the likely reading of the existing docs. Thoughts?

@burtek
Copy link
Copy Markdown
Contributor Author

burtek commented Jan 12, 2024

Kinda feel like deprecating (while still supporting) old config, but making new config be:

{
    between: 'force' | 'force-one' | 'prevent' | 'ignore' // between children
    adjecent: 'force' | 'force-one' | 'prevent' | 'ignore' // before first child and after last child
}

force - must be at least 1 empty line
force-one - must be exactly 1 empty line
prevent - must be 0 empty lines
ignore - ignore case

Obviously if both are ignore then rule does nothing.

Those properties might probably be called something else (better?).

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Jan 12, 2024

"between" seems weird for a parent/child relationship. Are you just trying to think of possible combinations, or do you have use cases?

A linter saying "adjacent" usually means lexically, not like "siblings" in terms of the AST graph.

@burtek
Copy link
Copy Markdown
Contributor Author

burtek commented Jan 12, 2024

So I want to distinguish all four options I saw in different projects:

<parent>
    <childA />
    <childB />
</parent>
<parent>
    <childA />
    
    <childB />
</parent>
<parent>

    <childA />
    
    <childB />
    
</parent>
<parent>

    <childA />
    <childB />
    
</parent>

Easiest way would be allow configuring the rule for newlines between siblings separately from newlines before first and after last sibling. between and adjecent are names that came to my mind, but as non-native speaker I'm not sure what those names should actually be.

And the choice between force, force-one, prevent and ignore is just all the possible options that came to my mind, based on current config scheme

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Jan 12, 2024

OK, let's aim for those 4 options, with an eye for future extensibility.

That means two keys, one for siblings and one for directly inside of a parent, each of which a "must have a newline" value, and a "must not have a newline" value. I suppose we could also have "ignore", too.

What about betweenSiblings and aroundChildren?

(allowMultilines would apply to all permutations)

@burtek
Copy link
Copy Markdown
Contributor Author

burtek commented Jan 12, 2024

Sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: react/jsx-newline does not remove empty lines for surrounding tags

2 participants