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

RFC: Deprecate array prototype extensions #1

Closed
wants to merge 12 commits into from

Conversation

smilland
Copy link
Owner

@smilland smilland commented Apr 1, 2022

We are moving away from prototype extensions.
Rendered

Copy link
Collaborator

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started reviewing, will review further a bit later in the month!

text/0000-deprecate-array-prototype-extensions.md Outdated Show resolved Hide resolved
Co-authored-by: Chris Krycho <hello@chriskrycho.com>
smilland and others added 4 commits April 28, 2022 18:00
Co-authored-by: Chris Krycho <hello@chriskrycho.com>
Co-authored-by: Chris Krycho <hello@chriskrycho.com>
Co-authored-by: Chris Krycho <hello@chriskrycho.com>
@smilland smilland requested a review from chriskrycho May 5, 2022 06:19
 Please enter the commit message for your changes. Lines starting
Copy link
Collaborator

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Have a couple tweaks to do before landing it, but then you should open it on the Ember repo!


Rule `ember/no-array-prototype-extensions` is available for both [eslint](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-array-prototype-extensions.md) and [template lint](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-array-prototype-extensions.md) usages. Rule examples have recommendations for equivalences.

We can leverage the fixers of lint rule to auto fix some of the issues. We will also create codemods to help with this migration.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you done any research yet into what can and cannot be codemodded, and what the level of reliability of those codemods is? If so, that would be great to add here.

Copy link

@bmish bmish Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will a codemod have any runtime information available (beyond just the static analysis that's available in a lint rule)? If so, it would be more useful than just a lint rule autofixer. If not, or either way really, I'd love to see as much autofixing capability added to the lint rule ember/no-array-prototype-extensions as possible. Note that the lint rule employs a variety of heuristics to avoid false positives given the limitations of static analysis.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmish I don't think so, unless we did a telemetry-powered codemod like the native class codemod did. Which is doable, but also a lot more effort.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, then personally I think it'd be worth focusing on building out the lint rule autofixer, as that would be easily accessible to basically all Ember users who will already have eslint-plugin-ember installed and integrated into their IDE/tooling.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmish an auto-fixer is good, but it also has significant limitations: part of the trick here is the way these methods continue to exist on EmberArray. I am talking separately with the Framework team about whether we can deprecate or otherwise separate that out in the period between now and 5.0, because of exactly this complexity and entanglement. 😩

Copy link
Owner Author

@smilland smilland Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you folks, I just opened the PR in public repo, emberjs#848 let's move our discussions there! @chriskrycho @bmish

text/0000-deprecate-array-prototype-extensions.md Outdated Show resolved Hide resolved
@smilland
Copy link
Owner Author

Open in public

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

Successfully merging this pull request may close these issues.

3 participants