Feedback on some of Rome's linter rules

[nstepien]

useKeyWithClickEvents

I think it's a good rule in general, but it also flags code like the following, which I think are valid cases to only have click handlers:

<div
  onClick={(event) => {
    event.stopPropagation();
  }}
>

useSimplifiedLogicExpression

Probably the most perplexing rule for me

const a = !b || !c;

Rome suggest replacing this with

const a = !(b && c);

Is it really "simplified"?
Sure it's simpler to a machine, but I find it less human-readable,
in the original line I can read it left to right, in the suggested line I read it as "both b and c must be truthy, then get the opposite boolean value...".

Now replace b and c with actual variable names and it gets more difficult to read.

// can read as "is not dirty or is invalid"
const disabled = !isDirty || !isValid;
// maybe you can read it as "it's not dirty and valid" but it sounds like you're reading `!isDirty && isValid`.
// at least to me I'd have to pause to understand the expression, I think junior devs especially would prefer the line above.
const disabled = !(isDirty && isValid);

It might also make things harder to change/maintain, as I may have to unroll the parentheses and reapply negations, while being careful not to forget to swap the logical operator.

noUndeclaredVariables

This one doesn't handle other jest's globals for example, such as describe, it, test, expect, {before,after}{All,Each}, ...
It isn't useful in TS files at least.

noUnusedVariables

I wish it would ignore unused function parameters that precede a used parameter, for example:

export const mockApi = rest.post(
  'api',
  // req is unused
  (req, res, ctx) => {
    return res(ctx.json({ data: [] }));
  }
);

Prefixing req with an underscore is noise, and would require removing it if we start using it.
VSCode already fades the parameter to indicate that it's unused.

That could be an option perhaps.

Another option would be to allow unused destructured variables as a way to clone object with removed properties.

const { id, ...newObj } = obj;
return newObj;

It should also suggest removing unused errors in catch clauses

try {} catch (error) {}
// can become
try {} catch {}

useSingleCaseStatement

It requires braces around virtually all cases, as cases usually have break; + at least 1 more statement.
If it required braces only on cases with an assignment like the ESLint counterpart then I'd be happy.

noImplicitBoolean

Has this ever been an issue? I don't think I've ever seen someone complain about an implicit true.
Is it useful in non-TypeScript codebases perhaps?

I don't think I've ever seen prop={true} in an API documentation, for example:
From the React docs themselves: https://reactjs.org/docs/forms.html#the-select-tag

From the react-router docs: https://reactrouter.com/en/main/route/route#index

Even in pure html it is allowed:

<input disabled>

and Chrome's devtools:

We enforce the opposite with this ESLint rule: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-boolean-value.md

useBlockStatements

We use ESLint's curly rule with the multi-line setting, but Rome requires braces even on one-liners:

if (entity === null) return <>-</>;

I wish Rome had a similar option.

[MichaReiser]

@nstepien excellent writeup. You may want to create a dedicated comment for each rule so that the discussion around a rule can be threaded (similar to this discussion)

[nstepien]

Well, if you want to discuss something about a specific rule feedback, feel free to start a thread 😄

[MichaReiser]

That's fair!

[ematipico]

@nstepien about useKeyWithClickEvents

If you think about it, the example you pasted is the reason the rule exists, to begin with.

In your example, you want to stop the propagation of the event, but you need to control the propagation of the onKey* events because people with the keyboard don't trigger the onClick event.

[nstepien]

Mmh, maybe you're right.

you need to control the propagation of the onKey* events because people with the keyboard don't trigger the onClick event.

(space/enter does trigger the click event on buttons/checkboxes for example)

[MichaReiser]

Our accessibility rules tend to over-report because they don't consider aria roles yet. So the example may be because of that. We plan to improve the rules in the upcoming release.

[ematipico]

Yeah, we don't have all the rules in place yet, and some of the existing ones still need some fine-tuning. But with the correct rules, you would get:

1. a lint rule saying that a non-interactive element that has interactive events, needs to have an interactive correct role, aria-role="button" in this case
2. then a second rule that an element that has an interactive role needs to have onClick
3. finally, the onClick event needs to be paired with onKey* event

Eventually, they all lead to the correct result:

<div
	onKeyPress={stopPropagation}
	onClick={stopPropagation}
	aria-role="button"
>
Label
</div>

[ghost]

noUndeclaredVariables should have an exception list for global test variables (used with e. g. vitest)

"noUnusedVariables": {
  "level": "error",
  "options": {
    "except": [ "test", "describe", "it", "..." ]
  }
}

[AaronConlon]

@ematipico Hi.Can I disable useKeyWithClickEvents rule? Even if I am wrong.
I try add this config:

{
  "$schema": "./node_modules/rome/configuration_schema.json",
  "linter": {
    "enabled": true,
    "rules": {
      "recommended": true,
      "suspicious": {
        "lint/a11y/useKeyWithClickEvents": "off"
      },
      "style": {
        "lint/a11y/useKeyWithClickEvents": "off"
      }
    }
  }
}

But no works.Thanks.

[nstepien]

You'll have to configure it like this:
https://github.com/adazzle/react-data-grid/blob/cde71a661d1a0a9e07140dcad54cb3e04e75bf77/rome.json#L27

[ematipico]

Here's a link to the documentation: https://docs.rome.tools/linter/#disable-a-lint-rule

[AaronConlon]

Thanks.