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

[ban-types] banning 'object' in 3.0.0 discussion #2068

Closed
daviduzumeri opened this issue May 22, 2020 · 6 comments
Closed

[ban-types] banning 'object' in 3.0.0 discussion #2068

daviduzumeri opened this issue May 22, 2020 · 6 comments
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@daviduzumeri
Copy link

daviduzumeri commented May 22, 2020

So I've gone through the pull request that introduces it and the discussion therein -- and apologies in advance if I'm just whipping a dead horse everybody would prefer to stay dead -- but the recent 3.0.0 ban-types update's banning of object seems particularly ill-advised to me for a couple of reasons. We here at TouchBistro have our own custom lint ruleset so we can always just disable it, but I wanted to at least broach the subject in the general community about the specific usecases where object is useful because, well, there's always the possibility that I'm wrong.

In short, the recommended replacement of Record<string, unknown> doesn't, I think, generally replace what we use object for -- or, I suspect, what the vast majority of developers use it for. If there's another type that can be used as an extension base for generics to hard-lock that specific type as an object, I'd love to hear it, but Record<string, unknown> can't be used in this case because casting any ole' object type to that requires an index property, which adds a lot of complexity to the typing just to make lint rules pass. There's a larger discussion here about how perhaps TypeScript should make index properties default for keys of objects or provide greater flexibility for type refinement when interacting with object, but at the end of the day I don't think creating types with properties that are objects is all that rare -- take, for example, a database model with a jsonb column, where the format of what's in that column can change, So Model<T extends object> { name: string, id: number, blob: T } allows for sub-typing of that model. The argument is made that this is a niche case, but is it really that niche? If it is, then I'll totally take the L on this one and acknowledge that we just have different needs in the ruleset, but unlike Object or Function, there are perfectly cromulent uses of object that, while maybe a bit advanced, certainly don't take it out of the realm of acceptable idiomatic TypeScript -- and moreso, I'd argue that even MORE niche are the cases where it can be acceptably replaced with an unsealed indexed type such as Record<string, unknown>.

Look, I love Record -- there are tons of situations where it's a wonderful tool, especially when performing diffs or other in-the-guts object-level wizardry or whatever -- I'm not knocking Record. And I recognize that the goal of the lint ruleset is to keep dangerous tools out of the hands of developers who might not realize their implications. But object isn't a handgun, it's at best a Swiss Army pen knife, and I'm not sure putting rubber on it and suggesting a nail file instead is the proper risk/reward calculus.

@daviduzumeri daviduzumeri added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 22, 2020
@bradzacher
Copy link
Member

#848 (comment)

... 99.9% of the time, you don't want to use it, and the vast majority of codebases won't want to use it. IMO that makes it a great candidate for banning by default.
In the 0.1% of the cases, you can use a disable comment and clearly justify why you're using the type, or you can override the default config (should have a better config solution for the 3.0 release).

We build the recommended set and default rule configs for the majority of users, not just power users.

Most users will not want to use it. For the most part it's a bad type to use because it's nigh impossible to use due to microsoft/TypeScript#21732.

I do recognise that there are perfectly valid use cases for object, and depending on what your project is, those uses may range from "never ever used" to "use frequently".

For example;

  • A project like a utility library that provides a general-purpose API to consumers, will likely have frequent uses for the object type to allow consumers to pass any object in. That's perfectly valid and likely to be frequent throughout the code.
  • A project that's purely an application will likely have strict types all the way through the codebase, so introducing an object into that stack will likely be problematic and potentially unsafe. So in those cases using object becomes the 0.1% case.

The vast majority of our users fall into the latter category - they are working at some company building out a product such as a website.

If you fall into the first category, then you can reconfigure the rule as appropriate.
We don't currently have a means to just turn off one part of the default config - happy to accept a PR which adds in the ability to do something like { types: { object: false }, extendDefaults: true } to disable the default option for a specific type.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels May 22, 2020
@daviduzumeri
Copy link
Author

OK -- that lines up with my understanding, at least; if our usecase of needing it as a base for a generic is really that specific in the grand scheme of things, I'm happy to run with that, I just wanted to make sure I wasn't fundamentally misunderstanding the situation. If users really are running into trying to use it as an actual object type when writing business logic, then yeah, Record is obviously a superior alternative. You've obviously got more of the data regarding what the acutal in the field use is, so if that's the case that makes perfect sense and maybe we'll even benefit by having to limit use of object in the wild as is. It's a shame that the in situation isn't resolved since being able to type refine an object using that would really be the best of all worlds, but c'est la vie :(

@bradzacher
Copy link
Member

Definitely - if in worked, I'd recommend object instead of Record<string, uknown> everywhere, as it gives you the absolute safest experience then.

But without that, object is pretty useless. This also sucks because unknown refines to object via a type foo === 'object', so it makes unknown a pain in the ass to use as well.

But I still recommend unknown as it's the only safe alternative to any.

@daviduzumeri
Copy link
Author

Oh yeah, any is the first brick on a slippery slope to a hell codebase, I've watched it happen. I guess I just figured our use cases for object -- for some data modelling stuff and also as a base extends for generics where basically we want to say "anything that's an object" -- were more ubiquitous in use than they actually are. Thanks a bunch for the conversation and checking my logic on this!

@photonstorm
Copy link

@daviduzumeri for what it's worth, I'm with you. ban-types is one of the first rules I disable entirely, because I resolutely fall into the 0.1% camp with you, building APIs where it's utterly impossible to know (or enforce) type structure in the inputs my APIs receive. I appreciate the default rules must be created for the 99.9%, although I'd put good money on the split not being quite that dramatic in practise.

@bradzacher
Copy link
Member

#2137 was released with the latest version which will allow you to specifically turn off the ban on just {}.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 3, 2020
@bradzacher bradzacher added wontfix This will not be worked on and removed awaiting response Issues waiting for a reply from the OP or another party labels Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants