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

[BUGFIX beta] Ensure proper .toString() of default components. #16307

Merged
merged 1 commit into from
Mar 1, 2018

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Mar 1, 2018

In older Ember versions the default components were generally labelled as Ember.Component or Ember.TextField via the NAME_KEY property on the class itself. During the glimmer-vm upgrade which landed during the 3.1 canary cycle the custom NAME_KEY was lost for Ember.TextField.

This commit removes needless usage of NAME_KEY (not needed since our normal Ember.Object instance's .toString methods check for the factories own toString properly), and updates the actual paths to match the new JS modules API locations (thus reducing more "globals" shenanigans).

In older Ember versions the default components were generally labelled
as `Ember.Component` or `Ember.TextField` via the `NAME_KEY` property on
the class itself. During the glimmer-vm upgrade which landed during the
3.1 canary cycle the custom `NAME_KEY` was lost for `Ember.TextField`.

This commit removes needless usage of `NAME_KEY` (not needed since our
normal `Ember.Object` instance's `.toString` methods check for the
factories own `toString` properly), and updates the actual paths to
match the new JS modules API locations (thus reducing more "globals"
shenanigans).
@mmun
Copy link
Member

mmun commented Mar 1, 2018

j/w where is this relied on?

@rwjblue
Copy link
Member Author

rwjblue commented Mar 1, 2018

CoreObject.prototype.toString uses the factories toString for the instances own toString display.

toString() {
let hasToStringExtension = typeof this.toStringExtension === 'function';
let extension = hasToStringExtension ? `:${this.toStringExtension()}` : '';
let ret = `<${this[NAME_KEY] || FACTORY_FOR.get(this) || this.constructor.toString()}:${guidFor(this)}${extension}>`;
return ret;
}

normally that means that we call makeToString on the resolver

toString() {
if (this.madeToString === undefined) {
this.madeToString = this.container.registry.makeToString(this.class, this.fullName);
}
return this.madeToString;
}

https://github.com/ember-cli/ember-resolver/blob/13f05eb45d2aef0fc860486f18e0897425362b43/addon/resolvers/classic/index.js#L131-L133

but these specific classes are not in the resolver

@rwjblue
Copy link
Member Author

rwjblue commented Mar 1, 2018

The reason I started digging into this (@rwwagner90 and I paired on this PR) was that the inspector had some tests that were trying to confirm that we had a valid string representation.

@mmun
Copy link
Member

mmun commented Mar 1, 2018

:shipit:

@rwjblue rwjblue merged commit d652f47 into emberjs:master Mar 1, 2018
@rwjblue rwjblue deleted the fix-display-for-text-field branch March 1, 2018 19:11
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