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

Update: add support for JSXFragments in indent rule (fixes #12208) #12210

Merged
merged 1 commit into from Sep 20, 2019

Conversation

kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Sep 3, 2019

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

fixes #12208

What changes did you make? (Give an overview)
This PR adds support to the indent rule for JSX Fragments, which are currently ignored. While working on this, I noticed that comments inside both JSX Element and Fragment tags are ignored if they match the indentation level of either the opening or closing tag delimiter. I've added tests to indicate that this is the desired behavior, but I'm thinking of proposing always indenting those comments one level because I can't foresee an instance when it would refer to anything outside the tag delimiters.

Is there anything you'd like reviewers to focus on?
I believe this matches the behavior of JSX Elements, but would love it if others can help me confirm!

It looks like both Babel's parser and Espree parse the following:

<>
< /* Comment */ 
    / 
/* Comment */ >

Does that seem correct? If so, I can add some tests.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Sep 3, 2019
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint indent Relates to the `indent` rule and removed triage An ESLint team member will look at this issue soon labels Sep 3, 2019
@kaicataldo kaicataldo changed the title Fix: add support for JSXFragments in indent rule (fixes #12208) Update: add support for JSXFragments in indent rule (fixes #12208) Sep 3, 2019
@kaicataldo
Copy link
Member Author

I believe this should be treated as a semver-minor bugfix and have updated the commit message to reflect that.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mysticatea
Copy link
Member

This looks a breaking change. I know several styles about the indentation of tag parts (e.g., the location of > sign).

@kaicataldo
Copy link
Member Author

This PR does increase errors, but it seems like a bug that Fragments aren’t treated the same way as other JSX tags.

@kaicataldo
Copy link
Member Author

@mysticatea Are you against releasing this in a non-major release? I personally would argue that this is a semver-minor bug because it feels like an unintentional oversight that the rule doesn't check JSXFragments, and that it just wasn't updated when JSXFragments were added to the JSX spec.

@mysticatea
Copy link
Member

Oh, sorry for my slow response. I had misunderstood because I had not noticed that the indent rule has supported JSX by default. I'm fine with a minor version.

@kaicataldo
Copy link
Member Author

No worries! Thanks for clarifying 😄

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Mar 19, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint indent Relates to the `indent` rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indent does not handle JSX fragment
4 participants