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

[no-extra-parens] Allow an exception for (new X()).y #12428

Closed
domenic opened this issue Oct 14, 2019 · 8 comments · May be fixed by rubarb666/rails#3, rubarb666/git-sketch-plugin#2, O330oei/node#4 or O330oei/node#11
Closed

Comments

@domenic
Copy link

@domenic domenic commented Oct 14, 2019

What rule do you want to change? no-extra-parens

Does this change cause the rule to produce more or fewer warnings? Fewer, if configured

How will the change be implemented? (New option, new default behavior, etc.)? New option, similar to the existing ones like returnAssign, nestedBinaryExpressions, etc.

Please provide some example code that this change will affect:

const doc = (new JSDOM()).window.document;

const { document } = (new JSDOM(`<!doctype html><html><head></head><body>
  <div></div><div title=""></div><div title="yes"></div>
</body></html>`)).window;

What does the rule currently do for this code?

It requires removing the parens, i.e. that I write

const doc = new JSDOM().window.document;

const { document } = new JSDOM(`<!doctype html><html><head></head><body>
  <div></div><div title=""></div><div title="yes"></div>
</body></html>`).window;

This is equivalent, but it feels really weird. Although the new operator binds more closely than the . operator in JS, this is a somewhat-surprising feature of JS, as . otherwise binds very high, and most space-separated operators bind quite low.

What will the rule do after it's changed?

Allow the above code, when a new option is configured, e.g."newExpressions": false.

Are you willing to submit a pull request to implement this change?

Yes, although a community volunteer would be much appreciated.

Other note

This changed in v6.5.0 as part of #12302.

@nzakas
Copy link
Member

@nzakas nzakas commented Oct 14, 2019

Thanks for reaching out. It does seem like we should have added an option for backwards compatibility when this rule was changed.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 14, 2019

I'm willing to champion and work on this.

@mdjermanovic mdjermanovic self-assigned this Oct 14, 2019
@kaicataldo kaicataldo added accepted and removed evaluating labels Oct 14, 2019
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Oct 14, 2019

This has now been accepted.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 14, 2019

Just to confirm the details of this enhancement, if I understand correctly the following should be done.

The option applies only to the change made in v6.5.0 (part of #12302):

(new X()).y;

(new X())[y];

Other cases of unnecessary parens around new X() that were warnings in previous versions will remain warnings, regardless of how the option is set. For example:

y = (new X());

f((new X()), y);

(new X())();

new (new X())();

The name of the option could be enforceForNewInMemberExpressions ?

Since it's default behavior in the actual version, the default value is true:

/* eslint no-extra-parens: "error" */

(new X()).y; // error

(new X())[y]; // error

y = (new X()); // error

f((new X()), y); // error

(new X())(); // error

new (new X())(); // error

When the option is explicitly set to false, (new X()).y and (new X())[y] are allowed:

/* eslint no-extra-parens: ["error", "all", { "enforceForNewInMemberExpressions": false }] */

(new X()).y; // ok

(new X())[y]; // ok

y = (new X()); // error

f((new X()), y); // error

(new X())(); // error

new (new X())(); // error
@mysticatea mysticatea added evaluating and removed accepted labels Oct 15, 2019
@mysticatea
Copy link
Member

@mysticatea mysticatea commented Oct 15, 2019

I have removed accepted label because there are three upvotes but there is a downvote from the team. This enhancement is a mixed reception and needs consensus.

My stance is neutral. It's hard for me to agree with the motivation to add the option because recent languages have the same precedence. But I know C++ is different.

About backward compatibility, I don't think it's a problem. This was a bug fix for the bug that no-extra-parens overlooked extra parens and our policy is that bug fixes that increase errors happens in minor releases.

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Oct 15, 2019

@mysticatea Michael is no longer on the team (see README). So his downvote is a community opinion, no more.

Would you like to 👎 this issue?

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Oct 15, 2019

Ah, Alumni isn't counted as the team's votes? In that case, I'm OK with accepted (I'm neutral on this issue).

@mysticatea mysticatea added accepted and removed evaluating labels Oct 15, 2019
@domenic
Copy link
Author

@domenic domenic commented Oct 15, 2019

Just to confirm the details of this enhancement, if I understand correctly the following should be done.

This makes sense to me. I think it's a reasonable question what the default should be (and thus what the name would be) but I'll leave this to the ESLint team.

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