Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat(eslint-plugin): apply final v6 changes to configs #7110
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
feat(eslint-plugin): apply final v6 changes to configs #7110
Changes from all commits
eb03fb4
632dee0
6e27cfc
d47cc0e
97f8815
380461d
e8855fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoshuaKGoldberg i don't remember our discussion here - why did we turn all of the options on?
I thought we discussed that turning all the rule options on was bad as it made the rule effectively useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue was that it was chafing a lot of users by being very strict by default. A lot of folks are intentionally e.g. logging template literal strings with various primitives inside.
This does bring up the idea of having different configs in the
strict
ruleset thanrecommended
... that would be a nice way to still get folks using the rule to its fullest if they want to be more strict. 🤔 #7318There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your example is template strings - but this is restrict-plus-operands.
So was this an accidental mix-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it applies to both of them. I've seen both happen with users. Which is why I keep mixing them up in comments 😅 to me, they're similar -just not exactly the same- cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the question is - why even have these turned on at all if all the options are on by default?
The rules enforce pretty nothing like this so it's just wasted lint time.
Eg for RPO there's the following cases. Of these 16 cases the default options mean the rule only matches FOUR cases - two of which are TS already errors.
So we're recommending a rule to everyone to stop them from doing:
string + object
andstring + unknown
. The former is also banned underno-base-to-string
, and one could argue the latter should be allowed under theallowAny
option.tl;dr - why are we even turning on the rules in the recommended set if they don't do anything with an overly permissive default option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple reasons swayed me:
string + unknown
no-base-to-string
without turning offrestrict-plus-operands
From a performance perspective, if one of these two rules is already asking for the types of these, I some level of caching on the TypeScript side would make the other rule not a significant change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing I'd point to is that we added this big caution block to the RPO docs because we didn't think people using the options was good - and yet it's the default that everyone uses these options?
https://typescript-eslint.io/rules/restrict-plus-operands#:~:text=%2C%0A%5D%3B-,CAUTION,-We%20generally%20recommend
Just seems like we're contradicting ourselves "don't use these options but also they're all turned on by default"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah :/ agreed. I don't like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference:
Given this rule is really doing nothing and a lot of people don't like it - we should just undo this change and only include it in
strict-type-checked