ES Classes #240
ES Classes #240
Conversation
|
Thanks for putting this together! I'm very much in favor... |
|
First things first: I just want to say this looks great. It's a well-written and well-thought-out RFC, and I'm 100% in favor overall! I do want to register a strong vote in favor of each of the alternatives raised as possibilities in the Alternatives section, with some reasoning supplied for why in each case. But I'll start by giving some context. Qualify All The ThingsWhile I profoundly appreciate Ember's strong use of conventions to accomplish things, I find both as a user and especially as someone trying to help others get up to speed on our app, I have a strong preference that things become more explicit when we're deploying new variants of existing APIs. The main complaint I hear from others on my team who come to our Ember app is that it's hard to understand what's going on, because there's so much that just magically happens. And that makes the learning curve harder. That goes especially for things like the way That sentiment goes double because I'm now the guy leading the charge with ember-cli-typescript: any magic we put in place makes it that much harder to fully support Ember from TypeScript. Now, a qualification to that sentiment is important: Even if we want TypeScript to be a first-class citizen of the ecosystem, right behind JavaScript, we should be careful not to let "easier for TS" dictate design decisions—even if we do want that to be a consideration. My point is simply that if we have two choices which are pretty comparable in terms of the JavaScript consumers' point of view but one is easier for TS, we should probably favor that. So take all that as background for my position on each of these things. Why the "Alternatives" are betterTL;DR: less magic in every case, and therefore fewer surprises for anyone reading the code, especially people coming from the broader ecosystem. In other words: all the reasons we're doing this in the first place militate strongly in favor of doing them in the way that provokes the least surprise. Class Property Initialization
Edit: see comment below; I changed my mind on this after @rwjblue's request for clarification below prompted me to re-read that section and think about it again. Merging and concatenating propertiesAs noted in my "qualifications" section above, I think these are already a rather surprising (albeit powerful) corner of the const FooComponent = Ember.Component.extend({
classNames: ['look-ma-a-foo'],
someOtherProperty: ['also an array', 'of strings'],
})
class BarComponent extends FooComponent {
classNames = ['hey-its-a-bar'];
someOtherProperty = ['these are', 'also strings'];
}Gladly, the docs cover this well:
Still, I think it's better to be explicit: class BarComponent extends FooComponent {
@concatenated classNameBindings = ['hey-its-a-bar'];
someOtherProperty = ['these are', 'also strings'];
}Explicit is better than implicit isn't a hard and fast rule, but it's a really good guideline for API design—and I think it's especially important when the behavior of one item is different from how everything else that looks like it behaves. Observers and listenersI take much the same view here as I do about the merged and concatenated properties. To those considerations, I would add mainly that I increasingly think the framework should discourage the use of both in favor of computed properties and normal actions for the majority of cases. And the docs seem to agree:
Given that preference, making these more explicit not only has all the wins expressed above, but it's also a slight bump in the work required to use them, and—most importantly—it makes it very explicit that the property is bound in the ways it is. SummaryGiven that the alternative proposals for merged/concatenated properties and observers and listeners are (a) backwards-compatible, (b) opt-in, (c) more in line with the way the rest of the JavaScript ecosystem behaves, and (d) easier to make sure we get right in TypeScript… well, those seem like pretty clear-cut cases to me. I think they're much better choices. And I think the alternative for |
|
@chriskrycho - Thanks for the detailed and thoughtful response! Believe it or not, I think that I actually agree with all very nearly all of your points, but I think the position this RFC is taking is still the correct course of action. Very specifically, this RFC is only focused on ensuring that I believe the correct course of action for those still overly magical things (merged and concatenated properties, some bits of evented, observers, etc) is to continue iterating in future RFC's to work through deprecations and proper replacements independently (for each feature). If we try to replace all of Ember's Object model in a single RFC we would never be able to proceed. |
|
@chriskrycho - I don't quite grok what you mean in the "Class Property Initialization" section. Can you explain in more detail (possibly with code snippets)? |
|
@rwjblue –
Totally agreed with that goal. My take was simply: if the alternatives work to support that option… let's do those; it's less to tackle in the future. That said, if the alternatives don't work to support that option, then I 100% agree: defer them, and if/when this RFC is accepted and implemented, write those as follow-ups.
No problem! This actually made me go back and reread that section again (and yes, that comes out to three readings!) and I actually changed my mind on reading it again. I think that section could perhaps be clarified just a bit: the idea comes out to *don't use |
|
So to be clear, The current proposal is that options passed to The alternative that @chriskrycho is mentioning is to instead update static create(args) {
const klass = new Class();
for (arg in args) {
klass[arg] = args[arg];
}
finishChains(klass);
return klass;
}So whatever would be passed to |
|
From a documentation and instruction side of things, while I agree that the main path should be to continue using I think including it in the official API as listed here and not documenting it would be a cause for confusion. So, my thoughts would be to add documentation for To clarify, this wouldn't be the pattern shown for the rest of the guides but the option would be documented in the Object Model section. |
|
|
@rtablada - While I am not opposed to a small paragraph or note suggesting that Ember's object model works with standard ES such as |
|
@rwjblue I'm not suggesting to update the whole guides as I had mentioned. Instead I was recommending a section under the |
|
One thing I'm not sure is quite covered here is the use of the I think that by introducing the idea of ES2015 classes it also makes users want to use the Clarification: I don't expect this RFC to make |
|
@rwjblue I was thinking after "Enumerables" so that is quick to reference.
|
Totally agreed. One thing to note here, however, is that our "legacy" is one of our strongest selling points: stability with an upgrade path to the future. If for example, we decided to drop support for object model features that are currently heavily depended on, many of our users would be completely stuck and unable to upgrade. It is very important that we do not bifurcate the community during these sorts of changes, which is why this is being handled in small digestible parts.
I totally disagree with you here. We are actually embracing what is done in vanilla JS for class properties. Consider the following: class Foo {
derp = "hi!";
constructor(value) {
console.log(this.derp); // => undefined
this.derp = value;
}
}
let foo = new Foo('bye!');
console.log(foo.derp); // => hi!As you can see in the proposal here (which was just moved to stage 3 this week actually) the constructor is ran before the class fields are applied to the instance. If you want to accept an argument but fallback to a default value, you simply have no choice but to use the
Agreed.
Great point! I believe that it would work properly with the changes listed in this RFC, but we should definitely state that in the RFC specifically. |
|
@rtablada (re #240 (comment)) - At this time we are not suggesting a change in Ember's DI system (e.g. Ember will continue to call the static However, with the changes listed in this RFC you would be able to use the following: class Foo extends Ember.Object { }
let f = new Foo({ some: 'prop', goes: 'here' });The main caveat (that we should continue to explore) is how we indicate to computed properties and observers that the user-land constructor is completed, and that we should begin listening to change events. This is currently done in the Moving the enabling of change events to the static Seems good to update the RFC to at least mention this. |
Absolutely, no question about that! There is a reason I joined the train somewhere @1.0.0-rc.x and never thought about leaving! 😉
Oh, was not aware of that, thanks for pointing that out! However I still think that does not rule out implementing return new Class(props);or like let instance = new Class();
Ember.setProperties(instance, props);
return instance; |
|
I'll also drop my view. I very much agree with @chriskrycho on the fact that I'd like to move concatenated properties, mixins, observers and listeners and other somewhat obscure features to some "official" decorator library. The reasons are the same: Less magic makes Ember easier to learn and more aligned with other frameworks. I don't fully buy the idea that In the same way that angle-bracket were added and then removed, and one of the reasons for that was because we wanted to take that unique opportunity of adding new way if invoking components to also change the semantics of components in a backwards incompatible way (for the better) and from such effort Glimmer.js was born, I think we could take the unique opportunity of switching from We could say "If you use I don't know if this might present some problem for interoperability. Options for interoperability are:
If we make the ES6 classes have less features, can then a user decide to use again I just want to highlight that this change in syntax offers the same opportunity to rethink our object model than angle-bracket components offered to introduce components with new semantics. I appreciate backwards compatibility as much as anyone else, but it's worth considering using this to break with the past in certain edge cases if that is for the best long term. In summary, I'd be confortable with making ES6 classes be "the object model of Ember 3.0" and the classical object model be maintained for compatibility. I don't think we will ever face a better syntactical change to improve the object model in a backwards incompatible way if needed. |
|
My only worry is that using |
|
@cibernox with the compatibility scheme for Ember I think it would have to be the Object model for Ember 4.0 to keep up with compatibility concerns. |
|
@NLincoln that's a good point that using |
|
I can't 👍 this enough. This change will improve IDE tooling and make Ember+typescript a much better fit. I'd like to add that method overrides should use ES6 export default Ember.Component.extend({
didInsertElement() {
this._super(...arguments);
}
});export default class extends Ember.Component {
didInsertElement() {
super.didInsertElement();
}
} |
|
We discussed this in the Ember core team call today. We are generally all in favor of this moving forward and decided to move this to final comment period. |
|
First off, I'm very 👍 on this. One topic not discussed here is Finally, I know the spirit in which it was meant, but I think we should stop short of calling Mixins, Observers, Listeners and Concatenated properties "obscure features" and "overly magical". It's one thing to call them advanced features, it's another thing to implicitly suggest they're bad and shouldn't be used. They've been very widely used. Observers: 1683 usages across 499 addons: Ember Observer |
|
Mixins are one thing. They have plenty of issues, for instance with ensuring call order for functions that are overriden (what does I agree that Listeners/Observers are not a bad pattern. When used correctly, they are incredibly powerful, and concepts like Promises/computeds do not always fully close over them. The issue here is that they come out of the box, standard on every object. This adds extra weight to every single Ember object, and encourages improper usage of a powerful, low level pattern. I disagree with concatenated/merged properties. They are highly magical, and like observers/events add weight to every object. However, unlike observers/events, they are very easily solved with a simple decorator, as in the example in the RFC. Overall, the argument (for future RFCs, not this one) isn't to take any of these features away. It's to split them out into explicit opt-ins, ones that could likely be shared with the larger Javascript community. |
Agreed, this was not mentioned. There are two different aspects that are important here:
To support this we will need to move more of the current object model to "native" ES likely meaning a move away from
Probably not. While I think it is important for interoperability that we make sure these methods work properly, I do not think
It seems clear to me that we must continue re-aligning the object model away from the custom bespoke system that we were forced to create when the underlying language lacked so many fundamentally required features towards leverage the common solutions that have been added to the language itself. It is quite likely that this migration will eventually mean some of our current object models features will need to be deprecated. We are absolutely not suggesting in this RFC that any new deprecations be added. Any new object model deprecations will almost certainly go through a deprecation RFC process. The specific terms "obscure" and "magical" are a bit subjective, if those terms seemed offensive to folks, I am definitely sorry. In my comment I was using "magical" to refer to features whose underlying implementation so complicated that even a core team member (me 😝 ) cannot understand them without a multi-hour deep dive... |
No, it definitely wasn't offensive. But they are very powerful and very useful features. In particular Mixins and Observers. These are features found in many languages and frameworks. I think they've gotten a pretty bad rap in Ember because they were incorrectly used in lieu of proper framework solutions (e.g. observing values before we had promises). "When all you have is a hammer, everything looks like a nail." |
Totally agreed on both accounts! RE: Mixins, this RFC is specifically suggesting that we should embrace native ES features (ES classes today do not support "mixins" like Ember has). |
|
If we get this, wouldnt this mean we can use mixing like this: |
|
@luxferresum Yes, that's equivalent to const BaseClass = Ember.Object.extend(myMixin);
class Foo extends BaseClass {} |
|
We discussed this at the Ember core team meeting today. Lets do it. 🎉 👏 |
|
is this normal the merged PR file is still named "0000-es-classes.md" ? This name positions de file at the top of the "text/" dir while it is the last PR that has been merged. Forgive me if this is a dumb question since I am not very familiar with the RFC's merge process ;-) |
|
You are right, I was supposed to update that before merging 😱. Updated in 0af0bbe. |
|
Ok not sure where to ask this, think this is probably appropriate. Using If have this Mixin - // promise-resolver.js
export default Mixin.create({})And I do this in my app // in my app
import PromiseResolverMixin from 'ember-promise-utils/mixins/promise-resolver';
let PromiseResolverObject = Ember.Object.extend(PromiseResolverMixin);
let subject = PromiseResolverObject.create();It works perfectly. But if we want to use Typescript or ES6 classes // promise-resolver.ts
export default class PromiseResolverMixin extends Mixin<PromiseResolverMixin> {}or // promise-resolver.js
export default class PromiseResolverMixin extends Mixin {}and then use the same code as above // in my app
import PromiseResolverMixin from 'ember-promise-utils/mixins/promise-resolver';
let PromiseResolverObject = Ember.Object.extend(PromiseResolverMixin);
let subject = PromiseResolverObject.create();We get the error I get it, that the class itself is not a Mixin instance. // in my app
import PromiseResolverMixin from 'ember-promise-utils/mixins/promise-resolver';
let subject = new PromiseResolverMixin()doesn't work also, this // in my app (in .ts file)
import PromiseResolverMixin from 'ember-promise-utils/mixins/promise-resolver';
class PromiseResolver extends Ember.Object<PromiseResolverMixin> {};
let subject = new PromiseResolver();doesn't work either. I am kinda stumped here, how best to use ES6 class definition for Mixins ? |
|
In short, you can’t—and I wouldn’t try! The semantics just don’t map in well at this point. I also strongly encourage people to avoid mixins at this point. Mostly they just cause pain. To use a mixin defined with import Component from '@ember/component';
import SomeMixin from 'the/path/to/it';
export default class MyComponent extends Component.extend(SomeMixin) {
// normal class definition
}Note that you may still see type errors when you do that in TypeScript – happy to talk about those in an issue on https://github.com/typed-ember/ember-cli-typescript or in the #topic-typescript room in the Ember Slack. |
|
Sounds fair. What is an alternative to Mixin then ?? I guess that's the correct approach ? |
|
@championswimmer I personally prefer to have Utils classes. For example, if you want to create something to archive a model, instead of create a mixin and do |
|
Also worth note: you often don't need classes at all to solve this. In our experience, most stuff that we use mixins for can be replaced by plain old modules and pure in/out functions. |
|
Where can one find more documentation about this? For example, when defining a model like import DS from 'ember-data';
const { Model } = DS;
export default class FooModel extends Model.extend({
// When do things need to go here?
prop1: DS.attr(),
}) {
// Will this work?
prop2 = DS.attr();
} |
|
@jacobq you can find the most up to date documentation in the ember-decorators project's documentation. There is no official Ember documentation about using native classes yet because native classes have not officially shipped, and there are still potential changes that could change the way native classes are used. See RFC #337 for more details. |
rendered