🐛 `style/noShoutyConstants` does not recognize multiple uses of a constant.

lgarron commented 1 year ago

Environment information

`npx rome rage` doesn't seem to terminate?

CLI version: v10.0.1

What happened?

Rome issues two style/noShoutyConstants errors for the following code, based on a real-world use case:

const REQUIRED = "REQUIRED";
const OPTIONAL = "OPTIONAL";

export const schema = {
    a: REQUIRED,
    b: OPTIONAL,
    c: REQUIRED,
    d: REQUIRED,
    e: OPTIONAL,
    f: REQUIRED,
    g: REQUIRED,
    h: OPTIONAL,
};

Playground link

Specifically, it says:

  ℹ You should avoid declaring constants with a string that's the same
        value as the variable name. It introduces a level of unnecessary
        indirection when it's only two additional characters to inline.

  ℹ Suggested fix: Use the constant value directly

This reasoning is faulty, because the constants are used several times each. If I apply the suggested fix, then the minified output increases by 50%[^1]. Further, there are additional downsides:

This rule discourages DRY practices that — in this case — are generally helpful or at least harmless.
In particular, it becomes less straightforward to change all instances of a constant value, especially if not every occurrence in a file is there for the same reason. (For example, one may be for test data that is regularly modified, and another may be for business logic that needs to stay the same even when the test data changes.) By contrast, the existing code sample above makes it trivial to do a semantically accurate rename of any of the constants.
Rome's "helpful" output is very verbose, since it lists all the uses of a such a constant. This is an intimidating amount of error text for anyone adapting an existing project to use Rome and trying to establish value in doing so.

[^1]: 127 bytes vs. 85 bytes using npx esbuild --minify src.ts. That's not taking compression into account, though.

Expected result

I would expect style/noShoutyConstants not to error (using its current reasoning) at most if a constant is used multiple times. Even for a single use: it is common to regularly change the number of uses of such a constant down to a single use and then later back up, so I would still find it undesirable to have any warning at all.

So I'd personally like to advocate for style/noShoutyConstants not to be included in the default/recommended rules.

Code of Conduct

[X] I agree to follow Rome's Code of Conduct

ematipico commented 1 year ago

I'll be honest, I think we should actually deprecate this rule and eventually remove it. The rule was created to cover some niche use cases, but it seems to make more harm - as @lgarron demonstrated.

MichaReiser commented 1 year ago

I'll be honest, I think we should actually deprecate this rule and eventually remove it. The rule was created to cover some niche use cases, but it seems to make more harm - as @lgarron demonstrated.

I don't think we have to remove a rule because of one bug and there's also the option to remove it from the recommended set.

mzbac commented 1 year ago

I would like to work on this issue! Let me know if you guys have any concerns.

rome / tools