Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

📎 Lint cases similar to no-dupe-keys #3731

Closed
jeysal opened this issue Nov 14, 2022 · 5 comments
Closed

📎 Lint cases similar to no-dupe-keys #3731

jeysal opened this issue Nov 14, 2022 · 5 comments
Labels
A-Linter Area: linter S-Planned Status: planned by the team, but not in the short term task A task, an action that needs to be performed

Comments

@jeysal
Copy link
Contributor

jeysal commented Nov 14, 2022

Description

#3562 provided the initial implementation of no-dupe-keys for #3364
However, ESLint's no-dupe-keys covers cases that the initial implementation does not cover yet.
Namely, keys that are differently formatted numbers (e.g. 1 vs 0x1).
These cases are documented in the test:

// valid for now
// ESLint already catches properties keyed with different-formatted number literals, we haven't implemented it yet.
({ 0x1: 1, 1: 2 });
({ 012: 1, 10: 2 });
({ 0b1: 1, 1: 2 });
({ 0o1: 1, 1: 2 });
({ 1n: 1, 1: 2 });
({ 1_0: 1, 10: 2 });
// This particular simple computed property case with just a string literal would be easy to catch,
// but we don't want to open Pandora's static analysis box so we have to draw a line somewhere
({ a: 1, ["a"]: 1 });

We can additionally think about whether we want to fail lint on a vs ['a'] too. However, my stance on this is that stopping short of computed property names is a good place to draw the line. Analyzing computed properties and linting a vs ['a'] specifically because there is a single literal in the computed property, but not linting a vs ['' + 'a'] or vs [a /* const a = 'a' */] because it's there seems like a much weirder place to draw the line 😅
I think we should not do it just because ESLint does it. Instead, I think we should have another rule that autofixes ['a'] to a.
@xunilrj @ematipico do you agree? (always a gamble if I figured out the right people to tag :P)

@jeysal jeysal added the task A task, an action that needs to be performed label Nov 14, 2022
@jeysal
Copy link
Contributor Author

jeysal commented Nov 14, 2022

Can someone label and if necessary add to #3358 or another appropriate umbrella please :)

@ematipico ematipico mentioned this issue Nov 15, 2022
29 tasks
@ematipico ematipico added this to the 11.0.0 milestone Nov 15, 2022
@ematipico ematipico added A-Linter Area: linter S-Planned Status: planned by the team, but not in the short term labels Nov 15, 2022
@ematipico
Copy link
Contributor

@jeysal

Can someone label and if necessary add to #3358 or another appropriate umbrella please :)

That's done

@xunilrj @ematipico do you agree? (always a gamble if I figured out the right people to tag :P)

You can use .github/CODEOWNERS as reference. You tagged the correct people :)

Overall, I agree with your idea to where we should draw the line. Actually, now that I think about it, why don't split the rule in three? As far I can see, there are some niche cases that ... yes, they eventually do the same, but their use cases can differ.

For example, cases where we check for number literals or computed properties. The check of number literals seems to me a niche case (personally , I never encountered such cases) for libraries that work with graphs, animations or rendering engines, and we could not recommend and people opt-in only if they want.

For computed properties, it's mostly for us to help our mental sanity, because the logic can be quite messy and complex.

@jeysal
Copy link
Contributor Author

jeysal commented Nov 15, 2022

@ematipico three rules would make sense to me. I think they should be something like

  1. no-dupe-keys (as currently implemented)
  2. simple-number-keys (autofix e.g. 0x1 to 1, safe)
  3. no-literal-computed-keys (autofix e.g. ["a"] to a, safe)

Is this what you envisioned too?

@jeysal jeysal changed the title 📎 Extend no-dupe-keys lint cases 📎 Lint cases similar to no-dupe-keys Nov 15, 2022
@ematipico
Copy link
Contributor

@ematipico three rules would make sense to me. I think they should be something like

1. `no-dupe-keys` (as currently implemented)

2. `simple-number-keys` (autofix e.g. `0x1` to `1`, safe)

3. `no-literal-computed-keys` (autofix e.g. `["a"]` to `a`, safe)

Is this what you envisioned too?

That's it! I am going to update the umbrella issue :)

@ematipico
Copy link
Contributor

Closing in favour of #3739 , where the new rules are tracked there

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter S-Planned Status: planned by the team, but not in the short term task A task, an action that needs to be performed
Projects
Status: Done
Development

No branches or pull requests

2 participants