What rules do the community tend to override? #1089
Comments
|
|
@jonathanong Thanks! I'd forgotten Can you elaborate on when |
|
when i have something like `${path}?${query}`just seems like more work |
|
Thanks - I've added a few of your examples to the OP. (and fixed your markdown, hope you don't mind) |
|
|
|
https://github.com/este/este/blob/master/.eslintrc |
|
@steida Thanks! Can you give me an example of your Also, fwiw, Why would you need to disable As for |
|
As for no-class-assign As for forbid-prop-types and no-unused-prop-types As for the rest. |
|
@steida thanks - for the prop types ones, please file bugs on eslint-plugin-react if you find you still need the overrides. for the no-class-assign one, it seems like you could do |
ljharb
changed the title
|
The only custom rule we're using currently is |
|
@jonase I think those are common and legit enough that if you'd like to make a PR to the base config, I'd be happy to add them. |
|
From Material-UI: 'no-console': 'error', // airbnb is using warn
'no-return-assign': 'off', // airbnb use error, handy for react ref assign.
'operator-linebreak': ['error', 'after'], // aibnb is disabling this rule
'react/jsx-handler-names': ['error', { // airbnb is disabling this rule
eventHandlerPrefix: 'handle',
eventHandlerPropPrefix: 'on',
}],
'react/jsx-filename-extension': ['error', {extensions: ['.js']}], // airbnb is using .jsx
'react/jsx-max-props-per-line': ['error', {maximum: 3}], // airbnb is disabling this rule
'react/no-danger': 'error', // airbnb is using warn
'react/no-direct-mutation-state': 'error', // airbnb is disabling this rule |
|
I have set the following at Reactabular: "rules": {
"comma-dangle": ["error", "never"], // personal preference
"prefer-arrow-callback": 0, // mocha tests (recommendation)
"func-names": 0, // mocha tests (recommendation)
"import/no-extraneous-dependencies": 0, // monorepo setup
"import/no-unresolved": [1, { ignore: ['^reactabular'] }], // monorepo setup
"no-underscore-dangle": 0, // implementation detail (_highlights etc.)
"no-unused-expressions": 0, // chai
"no-use-before-define": 0, // personal preference
"react/sort-comp": 0, // personal preference
"react/no-multi-comp": 0 // personal preference
} |
|
|
At my company, we've overridden some rules that don't play nice with Mocha:
Also:
|
|
@oliviertassinari Thanks! Most of yours are strengthening rules we don't have on/as errors. For ref assign, ref callbacks don't have a return value, so instead of @bebraw re no-underscore-dangle, JS doesn't have private properties :-/ hopefully you've read the recent guide section on them. re no-unused-expressions, see below. @broguinn arrow-body-style in mocha tests does do that, but i've found that using To all: regarding |
|
The interesting changes from our config is that we changed these to be able to use flowtype in a good way: You can find the config here. The other changes are mostly adding extra rules and disabling a few because we haven't always used airbnb as a base. |
|
@ljharb Yeah, I'm not using |
|
@relekang Thanks! re no-duplicate-imports, does Flow not support importing multiple things all in one import statement? |
|
@ljharb I am not sure, but I have not found a way to combine and into one line.
Nice 👌 Need to read the changelog closer. |
|
@relekang if that's true, i'd file that as a bug on flow :-) |
|
Thanks for soliciting community feedback @ljharb! We override the following rules (that fall outside of the aesthetic preference category):
|
|
i'd argue that accepting duplicate import paths when one is |
|
It looks like there was an issue to support There is an open issue about allowing exporting types from import/prefer-default-export: import-js/eslint-plugin-import#484 |
|
@ljharb @lelandrichardson @lencioni I did not mean to say that the rule from the import plugin did not work. I tried to say that the rule no-duplicate-imports from eslint itself gives what I would think of as false positives with flowtype imports. Is there a reason that both are in use here? It looks like the one from the import plugin covers the cases that the one from eslint does. |
|
@relekang @lencioni @lelandrichardson since |
|
@QuentinRoy node can never be consistent with the browser in this way because the browser has out-of-band metadata via the |
node is trying to become more like a browser, they are looking into adding more browser related stuff into node like never before. ESModules, URL, URLSearchParams, Blob/File, whatwg streams, ppl start using node-fetch and typed array more and more in node just to be consistent with writing cross platform application and not having to include the hole node buffer into the web that takes up lots of scripting.
Disagree, writing cross node/deno/web applications is a better best practice that outweighs yours |
|
@ljharb I do not think this is the point. Node does not need MIME types to require the use of the full path rather than cropping out the extension. I agree about the module problem though (I wished they were designed differently in the first place). Hopefully import maps may help solving this. I am not quite sure what is the status of this proposal. |
|
Not using |
|
|
@yairEO unrelated, but you want |
|
We use ternary heavily: export default function SomeTab() {
const { data, error } = useSomeRequest();
return (
<>
{error ? (
<Result
status="error"
/>
) : data === undefined ? (
<Spin />
) : data.length === 0 ? (
<Empty />
) : (
<div>
{data.map((log, i) => (
<div>{log.message}</div>
))}
</div>
)}
</>
);
}
|
|
I found it hard to use storybook template args ( |
|
@xiaoxiangmoe This is probably just personal preference, but I usually go for either early returns or short-circuiting rather than nested ternaries in those kinds of scenarios. |
|
@vdh We also use ternary in const variable assignment, which can't early returns. |
@xiaoxiangmoe Personally as @vdh mention I would also prefer something like this 👇 based on your example. export default function SomeTab() {
const { data, error } = useSomeRequest();
if (error) {
return <Result status="error" />;
}
if (data === undefined) {
return <Spin />;
}
if (data.length === 0) {
return <Empty />;
}
return (
<div>
{data.map((log, i) => (
<div>{log.message}</div>
))}
</div>
);
}As for your point on const variable assignment, the examplanation here is good enought for me. |
|
Ternary is poor man's pattern matching. export default function SomeTab({ data }) {
return (
<div>
{data.map((x, i) => (
<div>
{x.type === 'foo' ? (
<Foo x={x.foo}></Foo>
) : x.type === 'bar' ? (
<Bar>{x}</Bar>
) : x.type === 'baz' ? (
<Baz baz={x}></Baz>
) : (
<div>{x.message}</div>
)}
</div>
))}
</div>
);
} |
|
@xiaoxiangmoe as a more debuggable and arguably cleaner alternative: function FooType({ x: { foo } }) {
return <Foo x={foo} />;
}
function BarType({ x }) {
return <Bar>{x}</Bar>;
}
function BazType({ x }) {
return <Baz baz={x} />;
}
function DefaultType({ x: { message } }) {
return <div>{message}</div>;
}
const Lookup = {
foo: FooType,
bar: BarType,
baz: BazType,
default: DefaultType,
};
export default function SomeTab({ data }) {
return (
<div>
{data.map((x) => {
const C = lookup[x.type] || lookup.default;
return (
<div>
<C x={x} />
</div>
);
}}
</div>
);
} |
|
Another example: const productId =
process.platform === 'darwin'
? 110
: process.platform === 'linux'
? 115
: process.platform === 'win32' && process.arch === 'ia32'
? 121
: process.platform === 'win32' && process.arch === 'x64'
? 120
: undefined; |
const productIDs = {
darwin: 110,
linux: 115,
};
const win32s = {
ia32: 121,
x64: 120,
};
const productId = process.platform === 'win32' ? win32s[process.arch] : productIDs[process.platform];I've never seen a nested ternary that needs to be one. |
|
@thernstig nope, because this project doesn’t make decisions based on voting. Functions always belong declared before they’re used. |
|
Apologizes @ljharb I have must have misunderstood the purpose of it being listed in the main thread, but thanks for clarifying. |
|
I use |
|
|
@seyfer im confused; why would a linter ever be run in production? (tests shouldn’t have NODE_ENV set to production) |
|
@ljharb I have a custom git commit hook that does run before the production build. If there is a lint error, it will stop the build. |
|
That's something you check in CI before you allow merging; the production build is usually done only on merged code (after these checks are run). Also, clientside git hooks can be easily bypassed, so unless you run them on the server they're useless for enforcement. |
|
Thought I'd chip in: {
"import/prefer-default-export": "off" /* Expandability of named exports over pushing for unwanted default exports & API changes */,
"no-cond-assign": [
"error",
"except-parens",
] /* Allow clear & concise assignments in conditionals, when necessary */,
"no-void": [
"error",
{ allowAsStatement: true },
] /* Allows void-calling async functions from sync code, explicit void returns, shorthand expressions */,
"require-await": "error" /* Prevent refactoring mistakes and async creep */,
} |
this is for Staging. when the requirement is to have staging as close to production as possible, and there is not so much difference for a simple build, there is no need to use specific .env.staging env. or envs could be supplied differently, f.e. from ENV_* variables. I think you should not run CI/CD on the dev/local build, but as close to prod as possible. |
|
Linting is static analysis; it isn't run on a build, it's run on source files, which are identical everywhere. Your argument holds for tests, but not for linting. |
|
@ljharb my argument is for CI/CD in general, which usually does include code style checks, as well as other checks, such as complexity, etc. CI/CD needs to be automated, it runs f.e. with Jenkins or GitHub Actions, doesn't matter. And then, since it runs as one of the steps in the same workflow as tests and builds do run, they happen to use the same environment, and most often it is staging env (or prod, if there is no dedicated env). You can of course add dedicated staging, ci_cd or else name env and a config file per env, but that would be a bit more complex setup with the code duplication. It is easier, and actually makes sense, to use the same rules set in all envs for your project. And then, for a couple of rules, you don't want your build to fail with an error. Those couple of env checks are easier to add to the existing config. |
and others
I'm opening this issue specifically to gather a list of the rules that those in the community feel are controversial or problematic, such that they override them, and most importantly, why.
This is not an invitation for "+1s" (those belong as reactions on individual posts), or an invitation to hear about everyone's subjective aesthetic preferences, nor is it an indication that any of these rules necessarily will - or will not - change. I'm simply hoping to primarily gather civil, well-reasoned explanations for why certain rules are overridden, disabled, etc.
Any comments mentioning spaces vs tabs, or semicolons, will be summarily deleted :-)
We're aware of the following:
react/jsx-filename-extension- filed as Proposal: [react][breaking] Only .js files should contain JSX. #985. Why is it controversial? Some believe that.jsfiles should be allowed to contain JSX. Others believe this is an app-level concern, and does not belong in a shared config (like "env" settings).import/no-extraneous-dependencies: in test directories, for example, it needs to be overridden to the following (note: if/when eslint allows glob-based config, then we'd set up some common test directory patterns so you wouldn't have to do this manually)eslint-config-airbnbv13.0.0is now released, which resolves this.camelcase: when you're bound to the casing selected by APIs (What rules do the community tend to override? #1089 (comment), but Airbnb suffers from this too) This might also apply tonew-capandno-underscore-dangle.no-param-reassign: this happens frequently with areducewhere the entire operation is pure, but the reducer is not (ie, it mutates the accumulator, but the accumulator began as{}passed into the reducer). You can useObject.assign({}, accumulator, changes), or{ ...accumulator, ...changes }, however. (per What rules do the community tend to override? #1089 (comment))no-use-before-define: some enjoy using hoisting to define helper methods at the bottom of the file. This guide discourages relying on hoisting; instead suggesting importing the helpers from another file when possible. (per What rules do the community tend to override? #1089 (comment))no-mixed-operators- specifically where it relates to arithmetic operators, where the PEMDAS rule applies. We're definitely considering loosening this rule as it applies to*,/,+, and-.Any others? I'll update the original post with new examples as I'm made aware of them.
(Note: this does not cover env settings, like "browser", "node", "mocha", etc - these are app-level concerns, and as such, your
.eslintrc,tests/.eslintrc, etc should be defining them)The text was updated successfully, but these errors were encountered: