fix(eslint-plugin): [ban-types] add option extendDefaults

[threehams]

Special-case ban-types rule to replace default options instead of merging if extendDefaults is set to false.

Fixes #686.

[typescript-eslint]

Thanks for the PR, @threehams!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[bradzacher]

Thanks - this LGTM as it currently stands.

RC for this question - one thing I might ask is a new option.

type Options = [
  {
    types?: Types;
    extendDefaults?: boolean;
  },
]

Which would do the following: If false or omitted, it would work as you have just implemented - all or nothing. If true, it would do Object.assign(defaultOpts, userOpts.types) (i.e. extends and overrides the defaults with the user options).

This would save people copy-pasting the defaults all of the time, which would make it easier for them to include any future changes to defaults we do.

WDYT?

[threehams]

We might just be dancing around the issue of the Object message and fixer here. The primitive constructor type messages and fixers are a good default any project. Object, though, doesn't really have one universal way to fix it. Usage of it could be from someone really looking for behavior like object, or could be from someone not wanting to figure out the real structure of an object and just throwing in something that's effectively any. I've seen both usages in a Flow project.

The official recommendation is to use object which I'd agree with, but is probably the wrong fix in most cases. Record is a pretty unsound "just trust me" type even without the any mixed in, so we're just auto-fixing "any" to "almost any" there.

If the message were more generic and it didn't auto-fix, then this seems like a good use for the suggestions API, though it would require ESLint 6.7.

[glen-84]

This is related to #848, where the fixer for Object is removed (but Record<string, unknown> is still "suggested").

[threehams]

In that case, I'll add extendDefaults here since it'll be taken care of in that PR.

This change could take a little longer, it's not as clear how to do it without adding more one-off code in here. Maybe necessary though.

[bradzacher]

I think this is definitely a one-off use case. This rule is an edge case. I'm happy with the one-off code living in this rule.

Thinking about it, if you add extendDefaults with a default value of true, then this is no longer a breaking change - as it wouldn't meaningfully change it from the way it works currently.

[HolgerJeromin]

Thinking about it, if you add extendDefaults with a default value of true, then this is no longer a breaking change - as it wouldn't meaningfully change it from the way it works currently.

I think this would be the best option.
Friendly ping to @threehams :)

[threehams]

Updated code, tests, docs. Edited description as this is no longer a breaking change.

[bradzacher]

LGTM - thanks for seeing this through.

[codecov]

Codecov Report

Merging #1379 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1379   +/-   ##
=======================================
  Coverage   95.53%   95.53%           
=======================================
  Files         142      142           
  Lines        6625     6630    +5     
  Branches     1897     1900    +3     
=======================================
+ Hits         6329     6334    +5     
  Misses        106      106           
  Partials      190      190