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

[DEPRECATE] Deprecates aliasMethod #17540

Merged
merged 3 commits into from
Feb 2, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Jan 31, 2019

Deprecates the aliasMethod descriptor, and refactors it to not
use the Descriptor system internall

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

This is a little used function, but it is still public API. Do we need all public API deprecations to go through a deprecation RFC, or should it be based on relative usage?

@pzuraq
Copy link
Contributor Author

pzuraq commented Feb 1, 2019

I feel like it's extra overhead for something as small as this, without much extra benefit from the RFC process. Relative usage makes most sense to me, especially when you consider the opposite - a private API that is used throughout the community, for better or worse. High usage would definitely dictate a deprecation RFC for a private API, so miniscule usage for a public API IMO means we can skip the overhead.

@pzuraq
Copy link
Contributor Author

pzuraq commented Feb 1, 2019

Alternatively, we could streamline the process for these types of deprecations - Simplified RFC format, can be reviewed and merged without FCP, etc. Maybe create a threshold for usage that triggers requiring a full RFC.

Deprecates the aliasMethod descriptor, and refactors it to not
use the Descriptor system internall
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Can you update this to add svelte support? Specifically, that means to guard all code that isn't needed when the deprecation is gone (adding to @ember/deprecated-features and using an if).

@rwjblue
Copy link
Member

rwjblue commented Feb 1, 2019

We discussed this at the Ember.js Core Team meeting change today, and are 👍 to move forward without a specific deprecation RFC in scenarios that have very low usage and where we are extremely certain we understand all use cases.

tldr; lets land this (once the svelting work is added)

@pzuraq pzuraq force-pushed the refactor-alias-method branch from 76d30a8 to 29f5c2a Compare February 1, 2019 20:45
@@ -10,3 +10,4 @@ export const ROUTER_EVENTS = !!'3.9.0';
export const TRANSITION_STATE = !!'3.9.0';
export const COMPONENT_MANAGER_STRING_LOOKUP = !!'4.0.0';
export const JQUERY_INTEGRATION = !!'3.9.0';
export const ALIAS_METHOD = !!'4.0.0';
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong (as is COMPONENT_MANAGER_STRING_LOOKUP above) the value should be the version that introduced the deprecation (not the version that will remove it).

It should be (!!'3.9.0')...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, gotcha

@rwjblue
Copy link
Member

rwjblue commented Feb 1, 2019

Thank you @pzuraq!

@rwjblue rwjblue merged commit 310c0db into emberjs:master Feb 2, 2019
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.

2 participants