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 possible issue with either no-extra-parens or indent #4229

Closed
Moeriki opened this Issue Oct 22, 2015 · 16 comments

Comments

Projects
None yet
8 participants
@Moeriki
Copy link
Contributor

Moeriki commented Oct 22, 2015

Dears,

It seems like either one of these two rules is mistaken. I'm new to JSX so I'm not sure which one, if any.

Attempt 1
image

Attempt 2
image

Attempt 3 this fixes it actually. But I liked the first two best as style.
image

Your thoughts?

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Oct 22, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Oct 22, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 22, 2015

The parens issue has been brought up before (#3107) and we noted that those parens are extra, so if you want to use that style, you need to disable the rule around that area.

For indent - I think the question is whether or not return should imply an additional indent level or not. @gyandeeps @BYK

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Oct 22, 2015

I am not an expert in JSX, but in attempt 2 the return would only return undefined so u need braces.
As far as indent goes, indent is behaving correctly because anything below indent should align with return column wise because thats a separate statement not part of return at that time (this is proved aith attempt 3 as at that time the whole thing becomes a part of return).

My answer here is based on javascript in general and not JSX.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 22, 2015

Good points. Since this has come up a few times, maybe we should just add a "jsx" option of some sort to skip these?

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Oct 30, 2015

Parens around multi-line JSX seems to be the convention, and for that reason I typically disable no-extra-parens in JSX projects. I'd like to be able to re-enable it. The rule already has built-in exceptions for regular expression literals and IIFEs; would multi-line JSX have to be opt-in?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 30, 2015

Yeah, I'd expect it to be opt-in.

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Nov 14, 2015

Right now no-extra-parens accepts one option: a string, either "all" (default), or "functions". I propose adding a second option, an exceptions object. Right now it would have one possible key, jsx, with possible values "never" (default), "all", or "multiline". This is what it looks like added to the schema:

module.exports.schema = [
    {
        "enum": ["all", "functions"]
    },
    {
        "type": "object",
        "properties": {
            "jsx": {
                "enum": ["never", "all", "multiline"]
            }
        },
        "additionalProperties": false
    }
];

When the jsx exception is unset or set to "never", nothing changes, making this opt-in and non-breaking.

When the jsx exception is set to "all", the rule will not flag excess parens around any JSXElement.

When the jsx exception is set to "multiline", the rule will not flag excess parens around JSXElements that span multiple lines.

// Allowed by default/"never", "all", and "multiline"
return <foo />;

// Allowed by "all"
return (<foo />);

// Allowed by "all"
return (
    <foo />
);

// Allowed by "all" and "multiline"
return (
    <foo>
        Hello, world!
    </foo>
);

If this sounds like a good idea, I already have the implementation and tests ready.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 15, 2015

👍

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 16, 2015

I think a better option would be [no LineTerminator here], which allows parens around any multi-line expression following that spec production so it can be put on the following line.

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Nov 16, 2015

@michaelficarra To confirm my understanding, you're saying that rather than being specific to JSX as proposed, the rule should be made aware of the cases where the parens are significant and stop flagging those as gratuitous?

function foo() {
    return (
        someExpression
    );
}

Since the expression in the example is not on the same line as the return statement, the parens cannot be removed without changing the logic of the code. Right now, the rule effectively requires return values to be on the same line as the return statement. I don't know offhand where else [no LineTerminator here] applies, but it's doing the same thing there.

Is that right?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 16, 2015

@btmills Yep, that's right. Remember, it's only when an expression follows it, so the only productions are

ReturnStatement[Yield]:
  return [no LineTerminator here] Expression;
  return [no LineTerminator here] Expression[In, ?Yield];

ThrowStatement[Yield]:
  throw [no LineTerminator here] Expression[In, ?Yield];

YieldExpression[In]:
  yield [no LineTerminator here] * AssignmentExpression[?In, Yield]
  yield [no LineTerminator here] AssignmentExpression[?In, Yield]
@btmills

This comment has been minimized.

Copy link
Member

btmills commented Nov 16, 2015

@eslint/eslint-team I think @michaelficarra makes a great point here. Right now, the rule recommends a change (deleting parentheses) that could alter the function of the code if a developer either doesn't know or doesn't remember to move the start of the contained expression to the line above. In these cases, the parentheses are significant, and I don't think it should flag them. Such a change would be non-breaking, and I might go so far to classify this as a bug we could fix. Do you all agree?

@btmills btmills closed this in 169bd96 Nov 16, 2015

nzakas added a commit that referenced this issue Nov 16, 2015

Merge pull request #4440 from eslint/issue4229
Update: Add JSX exceptions to no-extra-parens (fixes #4229)
@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 16, 2015

😕

@nzakas nzakas reopened this Nov 16, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 16, 2015

Yeah, I think @michaelficarra's exactly right here. If we can solve this in a way that isn't specific to JSX, then let's do that.

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Nov 16, 2015

I'm going to do a PR to revert #4440 until we know exactly what needs to happen here. We may not need the JSX options at all.

@btmills btmills closed this in c411406 Nov 16, 2015

nzakas added a commit that referenced this issue Nov 16, 2015

Merge pull request #4450 from eslint/revert-4440-issue4229
Revert "Update: Add JSX exceptions to no-extra-parens (fixes #4229)"

@gyandeeps gyandeeps reopened this Nov 16, 2015

btmills added a commit that referenced this issue Dec 4, 2015

Fix: Handle forbidden LineTerminators in no-extra-parens (fixes #4229)
Previously, this rule would warn about excess parentheses even when they
were syntactically significant and removing them would alter the meaning
of a program. There are three applicable productions in the grammar with
the restriction `[no LineTerminator here]`:

```
ReturnStatement[Yield]:
  return [no LineTerminator here] Expression;
  return [no LineTerminator here] Expression[In, ?Yield];

ThrowStatement[Yield]:
  throw [no LineTerminator here] Expression[In, ?Yield];

YieldExpression[In]:
  yield [no LineTerminator here] * AssignmentExpression[?In, Yield]
  yield [no LineTerminator here] AssignmentExpression[?In, Yield]
```

If the expression following a `return`, `throw`, or `yield` is on a new
line, it must be wrapped in parentheses, and this rule should not warn.
It should, however, still check for double parentheses, since the second
set would be unnecessary.

Thanks to @michaelficarra for pasting the relevant portions of the spec.

btmills added a commit that referenced this issue Dec 5, 2015

Fix: Handle forbidden LineTerminators in no-extra-parens (fixes #4229)
Previously, this rule would warn about excess parentheses even when they
were syntactically significant and removing them would alter the meaning
of a program. There are three applicable productions in the grammar with
the restriction `[no LineTerminator here]`:

```
ReturnStatement[Yield]:
  return [no LineTerminator here] Expression;
  return [no LineTerminator here] Expression[In, ?Yield];

ThrowStatement[Yield]:
  throw [no LineTerminator here] Expression[In, ?Yield];

YieldExpression[In]:
  yield [no LineTerminator here] * AssignmentExpression[?In, Yield]
  yield [no LineTerminator here] AssignmentExpression[?In, Yield]
```

If the expression following a `return`, `throw`, or `yield` is on a new
line, it must be wrapped in parentheses, and this rule should not warn.
It should, however, still check for double parentheses, since the second
set would be unnecessary.

Thanks to @michaelficarra for pasting the relevant portions of the spec.

@btmills btmills closed this in 2d32f6e Dec 6, 2015

nzakas added a commit that referenced this issue Dec 6, 2015

Merge pull request #4612 from eslint/issue4229
Fix: Handle forbidden LineTerminators in no-extra-parens (fixes #4229)
@satazor

This comment has been minimized.

Copy link
Contributor

satazor commented Dec 26, 2015

Can we have @btmills commit cherry-picked to 1.x branch?

I'm interested in this fix but 2.0 is not yet released :(

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.