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

Fix jsx-wrap-multiline rule for closing parenthesis #194

Merged
merged 1 commit into from
Mar 12, 2019

Conversation

mpsijm
Copy link
Contributor

@mpsijm mpsijm commented Dec 22, 2018

There was a bug in this rule: having the opening parenthesis on a new line before the opening tag, but the closing parenthesis on the same line as the closing tag, would not trigger this rule.
This was caused by the fact that the index variable (as calculated by siblings.indexOf) was equal to -1 in some cases.
The index is now calculated by checking just the pos and end fields of a node, instead of the default object equality that indexOf uses.

With this change, the check that tested whether the node started with a newline is no longer needed (and with it, also the prevTokenKind variable is no longer needed).

Also, two tests have been added:

  • A multiline JSX component with the closing parenthesis on a new line, but the opening parenthisis on the same line
  • A multiline JSX component with the opening parenthesis on a new line, but the closing parenthisis on the same line

There was a bug in this rule: having the opening parenthesis on a new
line before the opening tag, but the closing parenthesis on the same
line as the closing tag, would not trigger this rule.
This was caused by the fact that the `index` variable (as calculated by
`siblings.indexOf`) was equal to -1 in some cases.
The `index` is now calculated by checking just the `pos` and `end`
fields of a `node`, instead of the default object equality that
`indexOf` uses.

With this change, the check that tested whether the node started with a
newline is no longer needed (and with it, also the `prevTokenKind`
variable is no longer needed).

Also, two tests have been added:

- A multiline JSX component with the closing parenthesis on a new line,
  but the opening parenthisis on the same line
- A multiline JSX component with the opening parenthesis on a new line,
  but the closing parenthisis on the same line
@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint-react, @mpsijm! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

nice, thanks @mpsijm

@adidahiya adidahiya merged commit f9ab7fb into palantir:master Mar 12, 2019
@adidahiya adidahiya added this to the 3.7.0 milestone Mar 12, 2019
@mpsijm mpsijm deleted the fix-wrap-multiline-parentheses branch March 13, 2019 18:57
@magic-akari
Copy link

Hello.
Is it expected behavior ?

const Test: React.FC = () => {
    return ReactDOM.createPortal(
        <details>
            <summary>test</summary>
            <div>
                Lorem ipsum dolor sit amet consectetur adipisicing elit. Fugiat numquam aliquid et cum quis consequatur
                commodi aliquam iure optio, ipsam voluptatum iste libero quaerat quia. Quos cupiditate eaque neque odio!
            </div>
        </details>,
        document.querySelector("#test"),
    );
};
ERROR: src/test.tsx:3:9 - Multiline JSX elements must be wrapped in parentheses

@mpsijm
Copy link
Contributor Author

mpsijm commented Mar 31, 2019

@magic-akari I'd say yes, this is expected. The <details>...</details> element is not wrapped in parentheses. The </details> tag is followed by a comma and a call to querySelector.

@magic-akari
Copy link

Thank you for your reply.

The parentheses I added will be erased by the prettier. And I think the parentheses are unnecessary in this case. I prefer the rule of prettier.

Since this is the expected behavior, I have no choice but to disable this rule.

@mpsijm
Copy link
Contributor Author

mpsijm commented Apr 2, 2019

Ah, you did not yet say that there is a conflict between the pretty-printer and the linter.
That does sound like a problem, maybe you can open a new issue describing your situation?

@adidahiya
Copy link
Contributor

prettier will often conflict with tslint & tslint-react's formatting rules. no need to open a new issue -- my recommendation is to stop using tslint & tslint-react's formatting rules and use prettier instead. the former will not be maintained in the future. see #210

falkenhawk added a commit to ovos/coding-standard that referenced this pull request Sep 6, 2019
because of false positives and being unmaintained (and being incompatible with prettier)
- palantir/tslint-react#194 (comment)
- palantir/tslint-react#79 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants