feat(eslint-plugin): split no-empty-object-type rule out from ban-types rule

[JoshuaKGoldberg]

BREAKING CHANGE:
Changes the default options of a lint rule

PR Checklist

• Addresses an existing open issue: fixes Enhancement: [ban-types] Split the {} ban into a separate, better-phrased rule #8700
• That issue was marked as accepting prs
• Steps in Contributing were taken

Overview

Per #8700 (comment), splits out the banning of {} from the ban-types rule into a new no-empty-object-type rule.

[typescript-eslint]

Thanks for the PR, @JoshuaKGoldberg!

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.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d33a246. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

🟥 Failed Commands
nx test eslint-plugin --coverage=false
nx run-many --target=lint --parallel --exclude eslint-plugin

✅ Successfully ran 28 targets

• nx run repo-tools:postinstall-script
• nx run types:build
• nx test eslint-plugin-internal --coverage=false
• nx test typescript-estree
• nx test visitor-keys
• nx test utils
• nx test scope-manager
• nx test type-utils
• nx test typescript-eslint
• nx test eslint-plugin-internal
• nx test repo-tools
• nx test parser
• nx test rule-schema-to-typescript-types
• nx test ast-spec
• nx test visitor-keys --coverage=false
• nx test ast-spec --coverage=false
• nx run integration-tests:test
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint
• nx test typescript-estree --coverage=false
• nx run-many --target=typecheck --parallel
• nx test parser --coverage=false
• nx test repo-tools --coverage=false
• nx test type-utils --coverage=false
• nx test utils --coverage=false
• nx test scope-manager --coverage=false
• nx test rule-schema-to-typescript-types --coverage=false
• nx test typescript-eslint --coverage=false
• nx run repo-tools:generate-configs

---

Sent with 💌 from NxCloud.

[JoshuaKGoldberg]

We didn't explicitly discuss Object, but I think it makes sense in the spirit of this change to switch it to being treated as another banned uppercase alias. I've never seen people confuse Object for {}.

[JoshuaKGoldberg]

FYI @mattpocock. 👋

Fun fact: this is the first time we're linking to totaltypescript.com on typescript-eslint.io... I'm honestly surprised it took this long! If there are other articles you think would go well in Further Reading sections for rules, we'd be very up for considering adding them.

[mattpocock]

Thanks pal, much appreciated!

[bradzacher]

Mildly tangential - but I can envision the following options:

• allowInTypeAliasWithName - for react codebases to allow type MyProps = {}
• allowInIntersection - to allow type T = A & {} - wherein the {} falls away.
  • does this even need an option...? Probably should be the default?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.34%. Comparing base (f4cbbe6) to head (39870f1).
Report is 29 commits behind head on v8.

❗ Current head 39870f1 differs from pull request most recent head d33a246. Consider uploading reports for the commit d33a246 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##               v8    #8977      +/-   ##
==========================================
- Coverage   87.35%   87.34%   -0.01%     
==========================================
  Files         386      387       +1     
  Lines       13019    13025       +6     
  Branches     3769     3769              
==========================================
+ Hits        11373    11377       +4     
- Misses       1355     1356       +1     
- Partials      291      292       +1     

Flag	Coverage Δ
unittest	87.34% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
packages/eslint-plugin/src/rules/ban-types.ts	95.83% <ø> (-4.17%)	⬇️
...es/eslint-plugin/src/rules/no-empty-object-type.ts	100.00% <100.00%> (ø)

[JoshuaKGoldberg]

@bradzacher threading #8977 (comment):

allowInTypeAliasWithName - for react codebases to allow type MyProps = {}

Isn't that a code smell? Why would they declare an empty props type? The only reason I can think of is copypasta / using a template, but it's "dead" code at that point.

[Josh-Cena]

@JoshuaKGoldberg It happens if you are using forwardRef and your component takes a second ref arg. Of course props: unknown would also work if you are not using it anyway...

[bradzacher]

You have two choices - declare nothing at all function Component() {} or declare an empty something function Component(_: Props) {}

A lot of people like doing the latter as it gives them an extension point to build from - eg if they know for sure they are going to add props later.

Forward refs is another good example of when it's hard required.

[JoshuaKGoldberg]

Why wouldn't you either omit the props type for forwardRef or use object?

[Josh-Cena]

Why wouldn't you either omit the props type for forwardRef

Because I use the following style:

function BaseComp(props: {}, ref: ForwardedRef<HTMLSpanElement>) {
  // ...
}

const Comp = forwardRef(BaseComp);

So I have to provide types.

use object

As I said unknown or object both work, but {} is not wrong, and best describes the intent (an object with more properties to be added).

[JoshuaKGoldberg]

best describes the intent (an object with more properties to be added).

But it doesn't describe just objects, right? It's an object or non-nullish primitive? I think in this case what we're seeing is the rule acting as it should: telling folks not to use {} because it also allows those primitives.

[bradzacher]

In the case of a react component - the type of the first argument is intersected with the standard framework provided JSX props by TS at the JSX "callsite". This is done automatically by TS - it intersects with global.JSX.IntrinsicAttributes IIRC.

EG if you defined function C(props: P) {} then render <C /> - the props you're allowed to pass to C are P & global.JSX.IntrinsicAttributes.

AFAIK all "JSX" frameworks define at least { key?: string } so the resulting type will ALWAYS be an object and not a primitive.

Additionally it's JSX - you can pass nothing but an object. JSX attributes are syntactic sugar for an object and a JSX spread attribute is enforced by TS to be an object.

So yeah you could do function C(_: unknown, ref: ForwardedRef<x>) {} - but that would be more verbose and more annoying than using _: {} or _: Props.

I'm all for us continuing to report on _: {} - but it's very common to do

type CompProps = {};
function Comp(_: Props, ref: X) {}

[JoshuaKGoldberg]

I'm still unconvinced 😄. The CompProps example is IMO even more in-violation because it (a) suggests doing that kind of type (= {}) is ok and (b) is something that can be reused outside the component. The fact that the framework / its types make the types safe is nice, but doesn't stop folks from reusing the dangerous type aliases outside the framework.

let myPropsToBeUsedLater: CompProps;

myPropsToBeUsedLater = "oops";

Is there maybe a more targeted option we could go for than the broad allowInTypeAliasWithName?

If someone explicitly says "Josh, we're in majority, we'd like to override your veto, please implement this" then I won't be upset at all. Nor would I be upset at all if you just push to this branch 🙂. Either way I'm expecting this rule to be one of the ones that gets a lot of scrutiny in the testing period for v8.

[Josh-Cena]

I feel like these two rules are really one, maybe as an option allowInterface for declaration merging

[JoshuaKGoldberg]

Hmm, I see them as separate rules. They're definitely related but I think the justifications for disabling either are very different.

[Josh-Cena]

interface T {} is not any different from type T = {} by itself, though. The motivation says:

An empty interface in TypeScript does very little: any non-nullable value is assignable to {}. Using an empty interface is often a sign of programmer error, such as misunderstanding the concept of {} or forgetting to fill in fields.

That is exactly applicable to empty object types. There is only slightly more reason to disable no-empty-interface because it allows declaration merging.

[JoshuaKGoldberg]

Interesting. I'll need to think on that a bit... Just confirming, you're suggesting we should merge no-empty-interface into this rule for v8, too?

[Josh-Cena]

Yes, exactly.

[bradzacher]

the difference between the two is that no-empty-interface allows you to ban interface Foo extends Bar {} in favour of type Foo = Bar.
However a similar rule doesn't make sense for type aliases.

But there's no reason we couldn't merge them and keep the allowSingleExtends option just for interfaces.

but you're JoshG isn't wrong - the scope is different because the current form bans {} anywhere - not just in type aliases.

I personally can't think of a reason you'd ever want to have one banned, but not the other.

[JoshuaKGoldberg]

Heh, I don't have a strong preference either way, and it sounds like the current consensus is leaning towards unification? Unless @kirkwaiblinger has strong input I'm up for unifying.

[kirkwaiblinger]

no strong opinions on this!

[kirkwaiblinger]

I will say, though, it does feel like yet another breaking change for users just to preserve the existing behavior. So, I guess "no strong opinion" defaults to not merging in this case 🤷

[bradzacher]

it does feel like yet another breaking change for users just to preserve the existing behavior

I would have said the opposite - it's a breaking change TO merge.
Cos if you don't merge - people are just switching from ban-types to this new rule.
But if you merge they ALSO need to switch from no-empty-interface to this new rule.

[kirkwaiblinger]

Big picture here looks good to me. Added a few more questions and suggestion, but basically consider this a thumbs up from me 👍

I want to say that I really like how you managed to keep the doc page concise on this nuanced topic, and encouraging about disabling if it helps more than hurts.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	d33a246
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/663b9c0843884800080348f1
😎 Deploy Preview	https://deploy-preview-8977--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 97 (🟢 up 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[bradzacher]

Also worth noting that we could land this as a standalone rule for v7 and then in v8 we change the defaults and make this new rule recommended.

That would allow us to do a deprecation of no-empty-interface this version and remove it in v8 - if we wanted to go that route.

[JoshuaKGoldberg]

standalone rule for v7

I like that a lot! Regardless of what we choose for no-empty-interface, this lets us get it released faster. Excellent. I'll get that PR up now. Edit: actually, no, I'll wait a bit - maybe it wouldn't make sense.

[bradzacher]

general logic LGTM