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.

Sign up for GitHub

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

Jump to bottom

add deprecation to NAME_KEY #16710

Merged
merged 1 commit into from
Dec 19, 2018
Merged

add deprecation to NAME_KEY #16710

merged 1 commit into from
Dec 19, 2018

Conversation

bekzod
Copy link
Contributor

@bekzod bekzod commented Jun 2, 2018

since NAME_KEY is still widely used https://emberobserver.com/code-search?codeQuery=NAME_KEY

bekzod
bekzod commented Jun 2, 2018
View reviewed changes
packages/ember-runtime/lib/system/core_object.js Outdated
@@ -608,7 +609,7 @@ CoreObject.PrototypeMixin = Mixin.create({
let hasToStringExtension = typeof this.toStringExtension === 'function';
let extension = hasToStringExtension ? `:${this.toStringExtension()}` : '';

let ret = `<${getName(this) || FACTORY_FOR.get(this) || this.constructor.toString()}:${guidFor(
let ret = `<${ this[NAME_KEY] || getName(this) || FACTORY_FOR.get(this) || this.constructor.toString()}:${guidFor(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted old behavior here, as some addon depend on it right now

@bekzod bekzod force-pushed the remove-name-key branch from 01ab5d5 to 1201c55 Compare June 2, 2018 10:38
@bekzod
Copy link
Contributor Author

bekzod commented Jun 2, 2018

@krisselden good idea ?

@bekzod bekzod force-pushed the remove-name-key branch from 1201c55 to bb17bcb Compare June 2, 2018 11:33
@rwjblue
Copy link
Member

rwjblue commented Jun 2, 2018

IMHO, nearly all of these cases would be better implemented via a .toString on the class/mixin/etc (which would be backwards compatible). Also, most of these addons use it speculatively, and we ensured they didn't error by continuing to have an Ember.NAME_KEY present.

tldr; unless we are aware of actual breakage, I'd rather not bring back the lookup for this[NAME_KEY]...

@bekzod
Copy link
Contributor Author

bekzod commented Jun 2, 2018 via email

@rwjblue
Copy link
Member

rwjblue commented Jun 2, 2018

Yes, that seems good to me, though it will need to tell folks to use .toString instead.

@bekzod bekzod force-pushed the remove-name-key branch from bb17bcb to 7ff3f8e Compare June 2, 2018 18:51
@bekzod bekzod changed the title add deprecation to NAME_KEY and expose setName add deprecation to NAME_KEY Jun 2, 2018
@bekzod bekzod force-pushed the remove-name-key branch from 7ff3f8e to d4ac71b Compare June 2, 2018 18:52
@krisselden
Copy link
Contributor

The usage of this.NAME_KEY in CoreObject instance method toString is likely never used, and the examples linked to are trying to add it to the constructor for classToString behavior. I don't remember why we did the NAME_KEY in there, I think it may have been how the factory at one point set name by passing it to create(). I'd like to redo the string stuff around just constructor.name and factory name, remove the namespace stuff.

@krisselden
Copy link
Contributor

I see, you want to propose setName be used instead of this pattern, we use constructor to string so if they implement a toString method on the constructor, it will be used instead.

@bekzod
Copy link
Contributor Author

bekzod commented Jun 28, 2018

anyway public usage should be deprecated right ? if not then you can close it

rwjblue
rwjblue requested changes Jun 28, 2018
View reviewed changes
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.

One minor tweak, but seems good to me after that...

packages/ember/index.js Outdated
get() {
deprecate('Using `Ember.NAME_KEY` is deprecated, override `.toString` instead', false, {
id: 'ember-name-key-usage',
until: '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.

Lets say 3.5.0 here, I don't think NAME_KEY was ever public API...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@bekzod bekzod force-pushed the remove-name-key branch from d4ac71b to c4667e0 Compare June 28, 2018 19:12
rwjblue
rwjblue approved these changes Jun 28, 2018
View reviewed changes
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.

Looks good, would you mind doing a deprecation-app entry for this?

krisselden
krisselden reviewed Jun 28, 2018
View reviewed changes
packages/ember/tests/reexports_test.js
['@test Ember.NAME_KEY is deprecated']() {
expectDeprecation(() => {
Ember.NAME_KEY;
}, 'Using `Ember.NAME_KEY` is deprecated, override `.toString` instead');
Copy link
Contributor

Choose a reason for hiding this comment

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

should make it clear that it is static toString. MyClass.toString = () => 'MyClass'

krisselden
krisselden reviewed Jun 28, 2018
View reviewed changes
packages/ember/index.js Outdated
@@ -202,7 +202,20 @@ Ember.canInvoke = utils.canInvoke;
Ember.tryInvoke = utils.tryInvoke;
Ember.wrap = utils.wrap;
Ember.uuid = utils.uuid;
Ember.NAME_KEY = utils.NAME_KEY;
Ember.setName = utils.setName;
Copy link
Contributor

Choose a reason for hiding this comment

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

if we prefer toString on the constructor, why are we exporting this?

Copy link
Member

Choose a reason for hiding this comment

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

Derp. I missed this when I last reviewed. Good catch @krisselden.

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this and it is good to go

krisselden
krisselden previously requested changes Jul 16, 2018
View reviewed changes
packages/ember/index.js Outdated
@@ -202,7 +202,20 @@ Ember.canInvoke = utils.canInvoke;
Ember.tryInvoke = utils.tryInvoke;
Ember.wrap = utils.wrap;
Ember.uuid = utils.uuid;
Ember.NAME_KEY = utils.NAME_KEY;
Ember.setName = utils.setName;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this and it is good to go

@bekzod bekzod force-pushed the remove-name-key branch from c4667e0 to c0917aa Compare July 17, 2018 13:31
Turbo87
Turbo87 approved these changes Dec 19, 2018
View reviewed changes
Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

rebased, and adjusted the deprecation warning to stay until v3.9.0

/cc @rwjblue

@rwjblue rwjblue merged commit 1032b80 into emberjs:master Dec 19, 2018
@bekzod bekzod deleted the remove-name-key branch January 8, 2019 07:33
@ghost ghost mentioned this pull request Feb 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

@Turbo87 Turbo87 Turbo87 approved these changes

@krisselden krisselden krisselden left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bekzod @rwjblue @krisselden @Turbo87