-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [ban-types] rework default options #848
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
Conversation
Thanks for the PR, @bradzacher! 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 |
I'm not sure if it makes sense to include Also, as mentioned in #842, I'm not sure if fixing the |
updated code and OP description. LMK your thoughts on this.
This is not a correct statement. The fact that they were all "use camelCase instead of PascalCase" was purely coincidental. |
It's definitely an improvement, although I'm still not 100% sure about
I meant that the defaults have always been about object wrappers, that's not to say that other objects cannot be used, just that it would be a (breaking, BTW) change to how things have been for quite a while now, and as I said, developers can always add this type to their configurations if they have no use for it in their codebases. |
I'm inclined to agree that this is a breaking change, so I've tagged it for v3 (which won't be miles away, we won't leave as big of a gap as we did between 1 and 2) |
@JamesHenry Maybe until then we can give a way to disable the default behaviour for Object type ? Should I do a PR for this? |
Have you considered adding
(no fixers) |
|
Yeah I haven't revisited this in a while, but both of those seem reasonable. For context of the types here: I really want the default config to reflect things you shouldn't use 99% of the time, because the 1% is easily covered by disable comments, with a supporting comment explaining why it's a reasonable usage. I would definitely argue that This is why For The problem is that I added a fixer with the original defaults, and I think there shouldn't be one for these types. "Basic" users probably want the fixer, as they meant an object type, but "advanced" users know what the type means, so they use it "correctly", so the fixer breaks their code. A very clear message explaining exactly what the types do is the best approach for them (same for function test_Record(obj: Record<string, unknown>) {
obj.foo.toString(); // ✅ Expected Error - obj.foo is of type unknown
if ('foo' in obj) {
obj.foo.toString(); // ✅ Expected Error - obj.foo is of type unknown
}
if (typeof obj.foo === 'object' && obj.foo) {
obj.foo.toString(); // ✅ No Error
}
if (obj instanceof Date) {
obj.getDate(); // ✅ No Error
}
}
function test_object(obj: object) {
obj.foo.toString(); // ✅ Expected Error - Property 'foo' does not exist on type 'object'.
if ('foo' in obj) {
obj.foo.toString(); // ❌ Weird error - Property 'foo' does not exist on type 'object'.
}
if ('foo' in obj && typeof obj.foo === 'object' && obj.foo) { // ❌ Unexpected Error - Property 'foo' does not exist on type 'object'.
obj.foo.toString(); // ❌ Unexpected error - same
}
}
function test_Object(obj: Object) {
obj.foo.toString(); // ✅ Expected Error
if ('foo' in obj) {
obj.foo.toString(); // ❌ Weird error - Property 'foo' does not exist on type 'object'.
}
if ('foo' in obj && typeof obj.foo === 'object' && obj.foo) { // ❌ Unexpected Error - Property 'foo' does not exist on type 'object'.
obj.foo.toString(); // ❌ Unexpected error - same
}
}
test_Record({}); // ✅ Expected No Error
test_object({}); // ✅ Expected No Error
test_Object({}); // ✅ Expected No Error
test_Record(1); // ✅ Expected Error
test_object(1); // ✅ Expected Error
test_Object(1); // ❌ Unexpected No Error
test_Record(true); // ✅ Expected Error
test_object(true); // ✅ Expected Error
test_Object(true); // ❌ Unexpected No Error
test_Record(null); // ✅ Expected Error
test_object(null); // ✅ Expected Error
test_Object(null); // ✅ Expected Error
test_Record(() => { }); // ✅ Expected Error
test_object(() => { }); // ❌ Unexpected No Error
test_Object(() => { }); // ❌ Unexpected No Error |
Did you mean I'm still a bit unsure about
... I assume that you mean: (obj as Date).getDate(); ... but you can also use function test(obj: object) {
if (obj instanceof Date) {
obj.getDate();
} else if (obj instanceof Location) {
obj.hash;
}
} |
Sorry, I just realised I messed up my example because TS only reported one error per expression. I've updated it so it's correct. But yes,
Instance of is definitely one way to refine it, but that only works when you have a well defined class structure, etc. But then if you're expecting a What I'm talking about expressly is the "I accept any object; anything that can use the string indexer and works correctly with |
I think the main difference between the two is that with It would be interesting to see actual use cases of both types.
It was just a contrived example to show the usage of |
Overall, personally, I wouldn't ban
function noviceCode(x: object) {
return (x as any).a + (x as any).b;
} And The narrowing thing is a shame, but In fact, it's specifically because I imagine this issue will get fixed eventually, (which would mean fixing And using And in general, banning The new (unreleased) handbook section on
As I mentioned above, it's rare that I think |
Which is the point I was making. 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. I agree though, that a fixer was the wrong approach. For both |
203b203
to
0aa09e1
Compare
Note: I have updated the PR based on feedback. |
Codecov Report
@@ Coverage Diff @@
## v3 #848 +/- ##
==========================================
- Coverage 93.91% 93.87% -0.04%
==========================================
Files 171 172 +1
Lines 7789 7804 +15
Branches 2240 2239 -1
==========================================
+ Hits 7315 7326 +11
- Misses 217 221 +4
Partials 257 257
|
8cd926d
to
b0e1a20
Compare
BREAKING CHANGE:
Updated the config based on feedback:
object
andObject
{}
Function
Fixes #842