eslint-plugin-ember icon indicating copy to clipboard operation
eslint-plugin-ember copied to clipboard

Remove `no-array-prototype-extensions` rule from `recommended` config

Open ef4 opened this issue 2 years ago • 2 comments

The no-array-prototype-extensions rule is trying to do an impossible thing. It's going to continue 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. I don't think it should be in the recommended set.

Examples:

https://github.com/ember-cli/eslint-plugin-ember/issues/1561 https://github.com/ember-cli/eslint-plugin-ember/pull/1552 https://github.com/ember-cli/eslint-plugin-ember/pull/1547 https://github.com/ember-cli/eslint-plugin-ember/pull/1546 https://github.com/ember-cli/eslint-plugin-ember/pull/1544 https://github.com/ember-cli/eslint-plugin-ember/pull/1543 https://github.com/ember-cli/eslint-plugin-ember/pull/1538 https://github.com/ember-cli/eslint-plugin-ember/pull/1539 https://github.com/ember-cli/eslint-plugin-ember/pull/1536

ef4 avatar Aug 10 '22 15:08 ef4

While we have been making a lot of progress addressing the false positives, it's true that there will always be false positives we cannot address for this rule. I'm open to removing it from recommended.

If anyone else has thoughts for or against, please comment and let us know.

bmish avatar Aug 10 '22 15:08 bmish

I'm in favor of removing it. It just has so many false positives.

mongoose700 avatar Aug 10 '22 15:08 mongoose700

Any other thoughts? @gilest @smilland

bmish avatar Aug 15 '22 19:08 bmish

Yeah happy to remove it.

It's good that the community has a heads-up about the upcoming deprecation.

The explantation and examples for this rule have been very helpful – perhaps they could be recycled into the RFC and eventual deprecation docs.

Agree, though, that code analysis is clunky, and the eventual in-code deprecation from ember-source will better serve the same purpose.

Appreciate the effort that went in to writing and maintaining it ❤️

gilest avatar Aug 15 '22 21:08 gilest

I'll proceed with merging and releasing this in the next few day unless there are objections.

I agree that even more important than the lint rule is for someone to write and implement an RFC to officially deprecate Ember array prototype extensions (similar to the previous function prototype extensions deprecation and string prototype extensions deprecation RFCs).

bmish avatar Aug 16 '22 21:08 bmish

Agree with removing from recommended rule. I am actually currently also working on drafting the ember RFC & deprecation guide. Will open under main repo soon. Thank you for all the contributions to the false positive fixes!

smilland avatar Aug 17 '22 02:08 smilland