Meta: enable more lint rules #3489
Conversation
|
Everything else looks good so far. Let me know when you’re done making changes, I might have other rule suggestions |
|
I am good except for my first comment |
|
Ok, can you sort all the rules them?
rules: {
/* Disable rules */
a: "off",
b: "off",
/* Add rules */
c: "error",
d: "error",
}I think in the future we should keep an eye on these 2 shared configs so we can restore the rules it disables or remove them from here if they're not necessary https://github.com/xojs/eslint-config-xo-typescript |
.xo-config.js
Outdated
| 'unicorn/no-fn-reference-in-iterator': 'off', | ||
| '@typescript-eslint/no-extra-non-null-assertion': 'error', | ||
| '@typescript-eslint/consistent-type-definitions':'error', | ||
| '@typescript-eslint/require-await': 'error', |
There was a problem hiding this comment.
There was a problem hiding this comment.
I remember this. It looks like the lint is failing because of this one.
If it works painlessly on the current code, we can enable it.
There was a problem hiding this comment.
This was the only one I was having issues with (hence my first comment)
.xo-config.js
Outdated
| 'unicorn/no-fn-reference-in-iterator': 'off' | ||
| 'unicorn/no-fn-reference-in-iterator': 'off', | ||
| '@typescript-eslint/no-extra-non-null-assertion': 'error', | ||
| '@typescript-eslint/consistent-type-definitions':'error', |
There was a problem hiding this comment.
While it might default to interface, this is not documented and also not clear. Please include the option explicitly.
Also, any reasoning to defaulting to interface?
The only benefit of interface is declaration merging, but type has other benefits too: https://stackoverflow.com/a/52682220
There was a problem hiding this comment.
but
typehas other benefits too
The only benefit I see is that you can use primitives, like type S = string, but this rule doesn't seem to prevent us from using that.
From the replacements I see in this PR, it only affects types that can as well be interfaces.
There was a problem hiding this comment.
The only benefit I see is that you can use primitives, like type S = string, but this rule doesn't seem to prevent us from using that.
Sure, but if you ever need that, you're back to inconsistency. Which is why I slightly prefer type, but it doesn't matter much. 🤷♂️
Kinda annoying that TS has such similar duplicate things.
There was a problem hiding this comment.
any reasoning to defaulting to
interface
I like definitions over declarations. The former looks cleaner to me:
interface X {}
type X = {};Kinda like I prefer function X() {} over const X = () => {};.
In the end, it's more about consistency than anything else. If they can be an interface, let them all be interfaces.
|
If you want to update the dependencies, this would be useful: fregante/webext-storage-cache#20 |
3 participants
@fregante I went through them all only
@typescript-eslint/require-await': 'error'am I not sure if they are correct.