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

Even more false positives with no-array-prototype-extensions rule #1561

Closed
simonihmig opened this issue Aug 10, 2022 · 2 comments · Fixed by #1748 or #1749
Closed

Even more false positives with no-array-prototype-extensions rule #1561

simonihmig opened this issue Aug 10, 2022 · 2 comments · Fixed by #1748 or #1749
Labels

Comments

@simonihmig
Copy link

On top of #1534, #1537 the rule also (IMO falsely) triggers when using methods of EmberArray, but not implicitly through prototype extensions (which is what this rule is trying to avoid AFAIU), but also when the EmberArray has been created explicitly, like in these cases:

  • A([...]).uniq()
  • blog.comments.toArray() (when blog is an ember-data model with a hasMany relationship - which also uses the Enumerable mixin)

While those cases maybe should be avoided in new code, they are not in any way deprecated (AFAIK), and as said above do not relate to prototype extensions, so they seem to be false positives here!

@bmish bmish added the bug label Aug 10, 2022
@bmish
Copy link
Member

bmish commented Aug 10, 2022

Yes, this is a false positive because it should be fine to use Ember array classes (EmberArray, MutableArray, ArrayProxy, fragmentArray from ember-data, others?) directly without relying on the prototype extensions.

We should be able to reduce false positives here by checking for simple cases where the object is created with A() or otherwise initialized to one of these Ember array classes. However, there will unfortunately be situations where the lint rule can't know what an object was initialized to (e.g. a function parameter, something imported from another file, etc).

ef4 added a commit that referenced this issue Aug 10, 2022
The `no-array-prototype-extensions` rule is trying to do an impossible thing. It's going to produce a never-ending stream of false positives. You cannot statically tell which calls are actually relying on the array prototype extensions.

A correct implementation is probably possible, but only in typescript (where eslint rules can take advantage of type information), not in javascript.

I'm all in favor of pushing people away from using array prototype extensions, but that is going to require runtime implementation. It cannot be checked statically. A rule that is so often wrong will just encourage people not to trust it anyway. I don't think it should be in the recommended set.

Examples:

#1561
#1552
#1547
#1546
#1544
#1543
#1538
#1539
#1536
bmish added a commit that referenced this issue Aug 18, 2022
…#1562)

* Remove no-array-prototype-extensions from recommended

The `no-array-prototype-extensions` rule is trying to do an impossible thing. It's going to produce a never-ending stream of false positives. You cannot statically tell which calls are actually relying on the array prototype extensions.

A correct implementation is probably possible, but only in typescript (where eslint rules can take advantage of type information), not in javascript.

I'm all in favor of pushing people away from using array prototype extensions, but that is going to require runtime implementation. It cannot be checked statically. A rule that is so often wrong will just encourage people not to trust it anyway. I don't think it should be in the recommended set.

Examples:

#1561
#1552
#1547
#1546
#1544
#1543
#1538
#1539
#1536

* docs: mentioned recommend rule removal in changelog

* docs: explain why no-array-prototype-extensions is not in recommended config

Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
@bmish
Copy link
Member

bmish commented Jan 17, 2023

Just to spell out one potential heuristic we could add for this related example:

async _fetchCategories(query) {
  const response = await this.store.query('connect/category', query);
  return response.toArray(); // false positive
}
class MyClass {
  async _fetch(query) {
    const response = this.store.peekAll('foo-bar', query);
    return response.toArray(); // false positive
  }
}
class MyClass {
  async _fetch(query) {
    return this.store.peekAll('foo-bar', query).then(response => response.toArray()); // false positive
  }
}

If a variable initialization comes from this.store, then we could assume it's Ember Data related and ignore it.

We should also call out in the doc any known false positives that we can't solve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants