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

eslint-recommended changes in eslint v6.0.0 #10768

Closed
aladdin-add opened this issue Aug 16, 2018 · 11 comments

Comments

Projects
5 participants
@aladdin-add
Copy link
Member

commented Aug 16, 2018

Similar to previous release, new rules can be added to eslint:recommended(https://github.com/eslint/eslint#semantic-versioning-policy)

some candidates:

  • require-unicode-regexp

  • no-misleading-character-class

  • no-async-promise-executor

  • require-atomic-updates

  • reconsider rules like eval, no-implied-eval, no-new-func. (#9406 (comment))

  • no-console

  • no-mixed-spaces-and-tabs: enable smart-tabs options.

  • no-shadow-restricted-names

  • no-prototype-builtins

  • #11279 (not accepted/implemented yet)

  • no-useless-catch

  • no-with

@aladdin-add aladdin-add added this to Needs discussion in v6.0.0 Aug 16, 2018

@not-an-aardvark not-an-aardvark moved this from Needs discussion to Accepted, ready to implement in v6.0.0 Jan 31, 2019

@not-an-aardvark not-an-aardvark moved this from Accepted, ready to implement to Needs discussion in v6.0.0 Jan 31, 2019

@platinumazure

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

I like all of the new candidates in the original post.

I would like to remove the following from eslint:recommended:

  • no-console

I would like to reconsider the following:

  • no-mixed-spaces-and-tabs: It would be ideal (IMO) if smart-tabs option were enabled in recommended. I would also be okay with removing this from recommended.
@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

I'd propose adding the following rules:

If #11279 is accepted and implemented before the next major release, I would also propose adding that rule.

I would lean against adding require-unicode-regexp because unicode regexes aren't supported in ES5. Adding no-misleading-character-class, no-async-promise-executor, and require-atomic-updates is fine with me.

I'm ambivalent about adding no-eval, no-implied-eval, and no-new-func because they do have niche uses (e.g. in simple REPLs and code generation logic). It could be argued that someone should only use this type of thing if they know what they're doing, and if someone knows what they're doing they should also be able to figure out how to disable the rule. On the other hand, I think false positives in eslint:recommended are very bad for UX, because if ESLint is reports too many spurious errors in the user's perspective, the user might be dissuaded from using ESLint or paying attention to its errors at all.

Along those lines, I agree that we should remove no-console from eslint:recommended.

@aladdin-add

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

agreed adding no-shadow-restricted-names, and removing no-console.

  • no-prototype-builtins: I am neutral on this case. we had some discussion in a TSC meeting.

furthermore, I'd love to propose:

  • no-useless-catch
  • no-with

@nzakas nzakas added accepted and removed evaluating labels Feb 28, 2019

@nzakas nzakas moved this from Needs discussion to Accepted, ready to implement in v6.0.0 Feb 28, 2019

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

In today's TSC meeting, the TSC decided to accept this change with specific rules to be decided on this issue.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

It seems like we need a full proposal to decide on. I'll propose the following:


Add the following rules to eslint:recommended:

  • no-misleading-character-class
  • no-async-promise-executor
  • require-atomic-updates
  • no-shadow-restricted-names
  • no-useless-catch
  • no-with
  • no-prototype-builtins

Remove the following rule:

  • no-console

aladdin-add added a commit to aladdin-add/eslint that referenced this issue Mar 17, 2019

@mysticatea mysticatea moved this from Accepted, ready to implement to Implemented, pending review in v6.0.0 Mar 27, 2019

@nzakas

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

I'm 👍 with everything except for no-prototype-builtins. I've always felt that rule was addressing an extreme edge case that most devs won't ever encounter and don't feel like we should be forcing it on our users.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Personally, I'm convinced that no-prototype-builtins is worth adding. I suspect a majority of hasOwnProperty usages are buggy even if no one has discovered the bug yet, due to the possibility that hasOwnProperty can be shadowed in user-provided objects. (hasOwnProperty is only useful for objects where the set of keys isn't known at development time, and that's precisely the case where one of the keys could shadow Object.prototype.)

To me, this is a case where developers can either (a) use a terse notation that is occasionally subtlely wrong, or (b) use a more verbose notation that doesn't have those problems. So my opinion is that always doing (b) is an easy choice, and enforcing against the buggy choice is important for developers who might not be aware of the subtle bug.

@btmills

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

I'm 👍 to all, though I won't argue too hard for no-prototype-builtins if there's not consensus around it. @not-an-aardvark makes a strong argument in my opinion because of the limited scope where hasOwnProperty is used. The two mitigating factors that would make me okay with not including it are that first, since it's a called function and not something that could be coerced to a boolean, it's less likely to result in a security vulnerability and more likely just a crash; and second, the use case for using an object as a map is going away now that we have built-in Maps.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Adding this to the TSC agenda because we haven't reached a consensus on the issue.

TSC Summary: We need to finalize a decision for which rules to add to eslint:recommended in v6.

TSC Question: What rules should be added?

@platinumazure

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

We've discussed in the 2019/04/11 TSC meeting.

Resolution: We will modify eslint:recommended as follows:

  • Add no-misleading-character-class
  • Add no-async-promise-executor
  • Add require-atomic-updates
  • Add no-shadow-restricted-names
  • Add no-useless-catch
  • Add no-with
  • Remove no-console

And we will discuss no-prototype-builtins in the next TSC meeting on 4/25. (Goal is to have a final decision on that rule with that meeting.)

v6.0.0 automation moved this from Implemented, pending review to Done Apr 26, 2019

not-an-aardvark added a commit that referenced this issue Apr 26, 2019

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented May 9, 2019

In the last TSC meeting, the TSC decided to add no-prototype-builtins to eslint:recommended in addition to the changes discussed in #10768 (comment).

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