-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Enhancement: [ban-types] Split the {} ban into a separate, better-phrased rule #8700
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
Comments
To be very clear here - the ban does not exist because we think it's an invalid type. And that statement alone shows that likely Ryan is reacting to peoe sending snippets to him, rather than the full docs or full error message. To make things absolutely clear - this is error message that users see when they use
We don't just ban it outright - this message has been very carefully constructed to lay all of the cards on the table and let the user make an informed decision. Additionally we don't ban it because we don't understand TS here and that we've labelled the type as invalid. No way no how. Instead the reason that we have a ban on People think it mens "an object with no properties" but it doesn't mean that at all. So the ban exists to warn people of this fact. People can easily disable comment if they really did mean "any non-null value" - but also IMO the actual cases where you mean that are 1 in a million. Put another way - when the average user writes So I'm a strong, strong 👎 to this. |
I'd be happy for this be split out into a separate rule entirely - as long as it was in the recommended set still. I firmly disagree that it's incorrect to flag There are some cases where we could not flag it because it does end up meaning "the right thing" (eg in an intersection type). But we can't add such handling into |
I still don't agree with this and it's still a constant source of frustration for me. It's still common for people to show up with one of these workaround types, complaining that they don't do what they want, when the thing they were using correctly in the first place actually was Lint rules are not a substitute for reading documentation, and they shouldn't try to be, either. They're there to prevent mistakes and enforce consistent style. It makes a lot of sense to ban
By this logic, we should also ban But you're not solving the underlying problem of users not knowing what
Banning I don't think it makes sense to see code littered with lint suppressions that just say "The person who wrote this code has read the docs", and the only workaround that should be recommended is
I don't understand this when one of the proposed workarounds is to use a type alias that itself uses |
I get that the confusion can exist, but also, you're going to see seems unlikely to conclude "Wow, TypeScript wrongly inferred that I just fundamentally don't agree with the idea that a type TypeScript will speak back to you ex nihilo, in uninteresting idiomatic code, is really so confusing that it has to be banned outright. People writing TS will see Orienting the default lint experience around a user who hasn't used TS for more than a few days is the wrong mindset to have when making this ruleset. It'd be bad to ban It'd be bad to ban It'd be bad to ban I could go on for days; users will misapprehend themselves of practically anything on even the most straightforward syntax. |
I'm in agreement with @RyanCavanaugh here. |
My personal experience working in the industry is that people do not read the TS handbook and they do not fully understand structural typing. Many people just start using TS and "figure it out". Often times people aren't afforded the time to read the docs and are instead thrown in the deep end and learn by doing. This isn't just true of TS's docs - the same goes for react docs, angular docs, internal company docs, etc etc etc. There are also a lot of people that started back in the day when the TS docs weren't as good as they are today as well. They often don't know the docs have changed and thus haven't updated themselves. I don't think it's fair to say "you must RTFM and there's no valid replacement for RTFM". Some people aren't able to learn by reading such walls of text. Some people don't have the time or energy to assign to reading such prose.
I fundamentally and whole-heartedly disagree. I could not disagree more. I have a lot of experience working directly with large bodies of engineers to try and teach them - it's been a substantial portion of what I've been doing for the last 5 years. This applies to all docs as I mentioned before and is not unique to programming language docs. No matter what you do - requiring people to RTFM is just not a viable way to disseminate and saturate information amongst your engineering cohort. In order to properly teach people the only way to do so is to do it live in the one place you have their undivided attention - in their IDE. The linter is ultimately the only tool that exists to do this. It's the only tool that's guaranteed to be integrated into an engineer's coding flow. |
Even when people don't read documentation and assume |
@Josh-Cena So if you're using it to mean "an OBJECT with exactly zero properties" you may be quite surprised to find that someone has passed a As a real example - it's not uncommon for backends to accept an empty object as a way of signalling true (instead of a boolean) in order to allow future extensibility of an option (eg it's easier to add properties to an object than it is to migrate callers from a boolean to an object -- one is a major breaking change and one is a simple minor change). So if you type your backend payload as This example is made particularly unsafe due to the across-boundaries passing of data, but one can imagine cases where people write code that assumes specific properties based on an assumption of "this thing must be an object with zero properties". It's an incorrect assumption and the lint rule is attempting to inform people that their assumption is wrong here. And to be clear - that's all it's doing. The error message makes ZERO mention of the type being invalid, unsafe, unsound or anything. It just says " |
I would also like to make it clear that whilst this is the default of the rule - the rule is completely reconfigurable. People can very easily turn off just this specific error message with this config: {
"@typescript-eslint/ban-types": ["error", {
"extendDefaults": true,
"types": {
"{}": false
}
}],
} We even explicitly show this case in the rule's docs. But people purposely choose NOT to reconfigure the rule. A lot of this is likely laziness as people don't want to bother changing their config. But I personally think that a lot of this is also because people agree with the error for all the reasons I've stated. |
Hmmm. Should I'd be happier if this is split out as a separate rule... It's informative but disruptive to most workflows, especially when developers can't easily modify the lint config. |
From my experience these types are actually fine. It's been a very very rare case that I've seen someone write Put another way - the intent of Scanning through some of the usages of this type in open source (of which there are very few) it looks like what I said is correct - the intent is to have some stringifiable value. I've never personally seen |
One thing that I think needs to be done here is to separate the power users from the average users. Power users are 110% on board with how structural typing works and how things. Everyone in this thread is a power user - Ryan quintuply so. There are two outs for such power users to step out of this behaviour: either disable comments or reconfiguring the rule. I'm always partial to the former because I like to explicitly document things so that future non-power-users can understand why something is safe. For example instead of disabling the type T<U> = { prop: string } & U extends SomeType ? { other: number } : {}; I would instead prefer to write: type T<U> = { prop: string } & U extends SomeType ? { other: number } :
// eslint-disable-next-line @typescript-eslint/ban-types -- this usage of {} does work as an "empty object type" as it is used in an intersection
{}; But I understand that in this regard I'm unique and most power users don't like to be so verbose because they believe that such a thing is obvious. For the average user - they likely aren't completely on board with structural typing. From my personal experience teaching people TS - it takes a lot to truly make them understand this or more importantly REMEMBER IT. Again - I spent a not insignificant portion of my job supporting other engineers. So often the answer to someone's question of "why didn't this do what I expected" was "because you didn't expect structural typing". Ryan you said "Orienting the default lint experience around a user who hasn't used TS for more than a few days is the wrong mindset to have when making this ruleset." but from my experience - people forget about structural typing! In so much code structural typing either doesn't really do anything or it does "what they expected" by acting as an inexact assignment. It's the rare cases where someone butts up against it that they are reminded of its existence. Which is a good thing - most people don't even need to consider structural typing most of the time! For the average user having a reminder that says "hey this thing probably isn't what you think it is" is a really good thing. But why special case |
I would also like to make clear here that I'm not married to the wording of the error or its suggestions. If it's seen as an unclear message - we can change this now in a minor version change. This wording is verbose and could be made more verbose. If we think we can clarify it further then LET'S DO IT. |
Best option for me: a separate rule. Second best: further clarify in the message that "if you actually intend to do this don't be afraid to add a disable comment or disable this particular type ban" But, we should definitely turn it off in |
I would like to say here that this code is very much a code golf strawman example. By-and-large people do not ever see a From my experience if someone is in an if (typeof p === 'object' && p !== null) {
p;
// ^? const p: object
}
Because from my experience few people actually "go to definition" on the TS builtin types to read their implementations. Additionally it wasn't until recently that this type itself used But that's beside the point - I don't quite understand why the implementation of
Because we're being complete. People complained in the past that they had usecase X, Y and Z that the error message didn't account for and they were upset that the error just said "dont use this type". So we changed the message to be a lot more verbose and attempt to act as documentation and cover all of the bases. We tried to enumerate all the usecases so that when the rule reported then people could understand the error and then remediate the error. But again - if you think there's more to be said as I mentioned lets make it better. |
Ryan I fully empathise with your support pain. Our intent is definitely not to push users to the typescript repo to complain that there's some weird behaviour and the linter doesn't align with it. You have an understandably negative sentiment towards this suggestion because it causes you support burden and you're sick of correcting users' misunderstanding of the message and the type. We can and should be doing better here. We should not be causing pain for you guys unintentionally. Though I would say that there's likely actions from both sides here. As an example the The fact that there's user want [1] for banning Similarly there's a gap on our side - we can improve the error message for people so that they don't misunderstand it and want to file issues in TS. [1] I say user want because banning |
As a typescript user, and specifically when I was new user, {} looked like it would behave similarly as a value and as a type. Same for []. I think the problem may lay somewhere that it’s not clear what It’s too much for a lint rule, but I think that in bigger code bases having type aliasing spirit of ObjectLike = {} could help. Then you could just disable a lint for that row and everybody would know when to use what (one can hope). —- I agree with everybody here. I think separate rule could be good. And maybe it should not be in recommended. But then, this is common mistake to make, so it would be good to be in recommended set. I think those types in typescript language are just little bit unfortunate. They are correct but I feel like they are technically correct. And maybe this time that is not the best kind of correct. |
I've read through this & the related threads a few times and I think the banning of
...but, almost no users do. So it's still a pain point even if users are "in the wrong".
I interpreted this as implying the two should work together, not that they're mutually exclusive. +1 to that. Documentation inherently cannot capture all the same nuanced things that linting does. If it tried, it would become so deep that nobody would be able to read it. So, +1 to the last couple paragraphs in #8700 (comment).
I think this matters quite a lot: many devs actually do read the built-in types. Personally I reference the files in my Learning TypeScript book (Chapter 11: Declaration Files > Library Declarations) and generally recommend folks read them as part of learning TypeScript when I teach it / give my workshops. I believe learning them is practically required to truly know the type system to learn them. By banning something that's used in a valid way in the built-in types, we add an additional complication to learning TypeScript well. We're saying "well, this is a bad practice, except in certain scenarios, which is used but not explained in the built-in types". Even if that's correct in some ways, it's confusing. We're now seeing a byproduct of that confusion in that people are reporting issues on TypeScript. Taking a step back: I think there are really two uses for the
Right now we're using Proposal...
Would this satisfy everyone? |
Regarding the 🍎 example above, |
@ehoogeveen-medweb that's what I was trying to imply with the name 😄 that the theoretical developer meant for it to be |
If a library declares a function taking In general I really don't know why users want to treat primitives so differently from objects, when such "object" cannot have any known properties anyway. The only difference would be (a) |
Hmm, yeah, I suppose this goes against the spirit of the |
Just wanted to add my two cents that I've seen multiple teams I've consulted for get confused by the This ends up introducing a bunch of weird index signatures into what should be simple mapped types. At a type-level, {} is the correct empty type for mapping over. Code-bases that lean heavily into mapped types are also much more likely to use I agree with @RyanCavanaugh that I would be very happy to see this toggled off by default so the focus of the rule can be shifted back toward types like |
@bradzacher, @Josh-Cena, @kirkwaiblinger, and I discussed internally. We're not all in complete agreement about #8697's moving the But, a compromise we're happy with for now is splitting out the ban into a new rule. This would give two benefits:
Renaming this issue to indicate it's for specifically the splitting Note that this does mean we plan to keep the |
Indeed. I hate this 😄. It's super incorrect+wrong behavior for "advanced" users to take anything some arbitrary tool -even one that seems official-ish the way we kind of do- at face value. The paradox of |
#8977 was merged into the |
Before You File a Proposal Please Confirm You Have Done The Following...
My proposal is suitable for this project
Link to the rule's documentation
https://typescript-eslint.io/rules/ban-types/
Description
Filing this issue as a followup/superset of the more targeted #8697. As of microsoft/TypeScript#49119, TypeScript has much better handling of
{}
semantics than it did when theban-types
default options were written. @RyanCavanaugh -the dev lead for TypeScript- called out in microsoft/TypeScript#57735 (comment) that as it stands today,{ }
is a valid type with a valid meaning in TypeScript.Proposal: let's re-think the default options in
ban-types
to agree with the way TypeScript now works?My first proposal would be, roughly...
{}
altogether, including inrecommended
andstrict
NonNullable<unknown>
that suggests switching to{}
...but I'd want to hear from @bradzacher and @RyanCavanaugh on whether that satisfies the intents of both TypeScript and
ban-types
.Fail
Pass
Additional Info
For clarity, the intent of the two issues I'm filing are:
ban-types
, which might take some timerecommended
experience, which can land firstIf this issue is resolved before #8697, that's great too.
The text was updated successfully, but these errors were encountered: