Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no-new-array confusing error message leading to new-for-builtins #2442

Closed
Abdullah-Rady opened this issue Sep 1, 2024 · 2 comments · Fixed by #2459
Closed

no-new-array confusing error message leading to new-for-builtins #2442

Abdullah-Rady opened this issue Sep 1, 2024 · 2 comments · Fixed by #2459

Comments

@Abdullah-Rady
Copy link

The eslint-plugin-unicorn has conflicting rules when trying to create a new array in JSX. Specifically, the rules unicorn/no-new-array and unicorn/new-for-builtins give contradictory advice, making it impossible to create a new array without an ESLint error.

When using new Array(repeat), I get an error from eslint/unicorn/no-new-array. If I remove the new keyword and just use Array(repeat), then I get an error from eslint/unicorn/new-for-builtins. This creates a confusing situation where neither approach is valid according to the current rules.

The rules unicorn/no-new-array and unicorn/new-for-builtins appear to be in conflict with each other.

const repeat = 5;

const MyComponent = () => (
  <>
    {
    Array(repeat) // This triggers `unicorn/no-new-array`
      .fill(0)
      .map((_, i) => (
        <div key={i}>
          Repeated Content
          {' '}
          {i + 1}
        </div>
      ))
      }
  </>
);

// Changing to the following code triggers `unicorn/new-for-builtins`

const MyComponentFixed = () => (
  <>
    {new Array(repeat) // This triggers `unicorn/new-for-builtins`
      .fill(0)
      .map((_, i) => (
        <div key={i}>
          Repeated Content
          {' '}
          {i + 1}
        </div>
      ))}
  </>
);

Expected Behavior

There should be a clear, non-conflicting recommendation for creating arrays in this context.

Environment

eslint-plugin-unicorn version: 55.0.0
Node.js version: 22.3.0
ESLint version: 8.57.0
@axetroy
Copy link
Contributor

axetroy commented Sep 2, 2024

unicorn/no-new-array should provide two suggestions that should fix the conflict

problem.suggest = [
{
messageId: MESSAGE_ID_LENGTH,
fix: fixer => fixer.replaceText(node, fromLengthText),
},
{
messageId: MESSAGE_ID_ONLY_ELEMENT,
fix: fixer => fixer.replaceText(node, onlyElementText),
},
];

It should be fixed to

Array.from({ length: repeat })
2024-09-02.13.48.18.mov

@fregante fregante changed the title Conflicting Rules - unicorn/no-new-array and unicorn/new-for-builtins no-new-array autofix triggers new-for-builtins Sep 3, 2024
@fregante fregante changed the title no-new-array autofix triggers new-for-builtins no-new-array direct opposite of new-for-builtins Sep 3, 2024
@fregante
Copy link
Collaborator

fregante commented Sep 3, 2024

Judging from the description of no-new-array, the rule is not about new, but specifically about the Array constructor. See:

When using the Array constructor with one argument, it's not clear whether the argument is meant to be the length of the array or the only element.

This is purely a documentation issue, the error should not be:

[MESSAGE_ID_ERROR]: 'Do not use `new Array()`.',

But something like:

  • Array constructors are unclear in intent, use either [x] or Array.from({length: x})

@fregante fregante changed the title no-new-array direct opposite of new-for-builtins no-new-array confusing error message leading to new-for-builtins Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants