From 94b38d429eb2964fa86cd13bea6823a01b3ef68d Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Thu, 14 Jun 2018 22:59:30 -0700 Subject: [PATCH 1/5] rfc text --- text/0000-native-class-constructor-update.md | 220 +++++++++++++++++++ 1 file changed, 220 insertions(+) create mode 100644 text/0000-native-class-constructor-update.md diff --git a/text/0000-native-class-constructor-update.md b/text/0000-native-class-constructor-update.md new file mode 100644 index 0000000000..903bf8e210 --- /dev/null +++ b/text/0000-native-class-constructor-update.md @@ -0,0 +1,220 @@ +- Start Date: 2018-06-14 +- RFC PR: (leave this empty) +- Ember Issue: (leave this empty) + +# Native Class Constructor Update + +## Summary + +Update the behavior of EmberObject's constructor to defer object +initialization. + +## Motivation + +Using native class syntax with EmberObject has almost reached full feature +parity, meaning soon we'll be able to ship native classes and begin recommending +them. This will do wonders for the Ember learning story, and will bring us in +line with the wider Javascript community. + +However, early adopters of native classes have experienced some serious +ergonomic issues due to the current behavior of the class constructor. The issue +is caused by the fact that properties passed to `EmberObject.create` are +assigned to the instance in the root class `constructor`. Due to the way that +native class fields work, this means that they are assigned _before_ any +subclasses' fields are assigned, causing subclass fields to overwrite any value +passed to `create`: + +```js +class Foo extends EmberObject { + bar = 'baz'; +} + +let foo = Foo.create({ bar: 'something different' }); + +console.log(foo.bar); // 'baz' +``` + +This has made adoption very difficult, and is a consistent stumbling block for +new users of native class syntax in Ember. Worse yet, it makes writing a codemod +for converting to native class syntax very difficult because we don't have a +clear target. + +For instance, given the above class, how would we convert the class field? Let's +go through the various options: + +```js +class Foo extends EmberObject { + // Does not work, for the reasons described above + bar = 'baz'; + + // Does not cover all cases. If we did `Foo.create({ bar: false })` it would + // still assign the default. + bar = this.bar || 'baz'; + + // This works, but is very verbose and not ideal + bar = this.hasOwnProperty('bar') ? this.bar : 'baz'; + + // This is one of the community accepted solutions, but it requires lodash + bar = _.defaultTo(this.bar, 'baz'); + + // This is another community accepted solution, but it requires + // @ember-decorators/argument, which is a separate library + @argument foo = 'bar'; +} +``` + +None of these is ideal. Instead, we can change the behavior of the constructor +and the `create` method to circumvent this issue. + +This change _would_ be a breaking change to the behavior of native classes +today, and a change from the previous class RFC. This will impact early adopters +and should be made with that in mind. However, because native classes never +officially shipped as part of Ember's public API (an announcement was not made, +docs have not been written, etc), this RFC proposes that the change would _not_ +be considered a breaking change _for the purposes of semver_. This would allow +us to ship the change during the Ember v3 release cycle, and prevent more code +from being built on top of the previous behavior. + +## Detailed design + +One very important design constraint to making this change is that we _cannot_ +break the behavior of EmberObject when used _without_ native classes. To do +this, we will leverage the fact that the static `create` method is the only +public way to create an instance of EmberObject. + +Currently, the behavior of EmberObject is the following (simplified): + +```js +class EmberObject { + constructor(props) { + // ..class setup things + + Object.assign(this, props); + this.init(); + } + + static create(props) { + let instance = new this(props); + + return instance; + } +} +``` + +We can change it to the following (simplified): + +```js +class EmberObject { + constructor(props) { + // ..class setup things + } + + static create(props) { + let instance = new this(props); + + Object.assign(this, props); + this.init(); + + return instance; + } +} +``` + +This would assign the properties _after_ all of the class fields for any +subclasses have been assigned. Revisiting our previous example, the following +two class declarations would effectively be equivalent: + +```js +const Foo = EmberObject.extend({ + bar: 'baz' +}); + +class Foo extends EmberObject { + bar = 'baz'; +} +``` + +Much easier to codemod! There are other subtle differences between native class +fields and EmberObject properties, such as the fact that class fields are +assigned each time a class is initialized, but these are easier to work around. + +### Injections and the `init` hook + +One side effect of this change is that injections will not be available on the +class instance during the `constructor` phase. The ideal behavior here is +outside of the scope of this RFC, and is something that should be discussed in +future RFCs. For the time being, users can still rely on the `init` hook, which +will continue to be called after all injections and properties have been +assigned to the instance. + +### `new EmberObject()` + +It was previously possible to use `new` syntax with EmberObject. While this +was not considered public API, it has technically worked and been under test +since the early days of Ember, and may fall under the category of intimate API. + +We can continue to support this behavior in a backwards compatible way while +deprecating it with one final tweak to the change above: + +```js +const DEFER_INIT = new Symbol(); + +function initialize(instance, props) { + Object.assign(instance, props); + instance.init(); +} + +class EmberObject { + constructor(props, maybeDefer) { + // ..class setup things + + if (maybeDefer === DEFER_INIT) { + return this; + } + + deprecate('using new with EmberObject has been deprecated', true); + initialize(this, props); + } + + static create(props) { + let instance = new this(props, DEFER_INIT); + initialize(instance, props); + + return instance; + } +} +``` + +## How we teach this + +If this PR is accepted, most of the major issues with classes will have been +resolved. We can begin working on a codemod to make converting easier, and move +toward officially making native classes a finalized part of the public API of +Ember. Pending decorators and class fields moving to a late enough stage in the +TC39 process, we can also begin converting the guides to use native class +syntax. + +We can document the exact behavior of the new constructor in the API docs for +EmberObject. Most details won't have to change since this change only affects +native class syntax, which has not been documented much officially. We can also +demonstrate the behaviors of classes throughout the guides and API docs. + +One thing we should make clear is that EmberObject will likely be deprecated +in the near future, and that ideally for non-Ember classes (things that aren't +Components, Services, etc.) users should drop EmberObject altogether and use +native classes only. For the purposes of codemodding, however, this will not be +possible, since EmberObject based classes still have many differences in common +usage from standard native classes. + +## Drawbacks + +This would be a breaking change that would negatively affect early adopters. + +## Alternatives + +We could leave the behavior as is, and choose a method for defaulting to +standardize on. + +## Unresolved questions + +How do we handle DI during the construction phase? From 64c6b0c856ea47f7c9f5fda7e50d3f555a873746 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Fri, 15 Jun 2018 07:38:25 -0700 Subject: [PATCH 2/5] updates --- text/0000-native-class-constructor-update.md | 30 +++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/text/0000-native-class-constructor-update.md b/text/0000-native-class-constructor-update.md index 903bf8e210..bc1764b3f2 100644 --- a/text/0000-native-class-constructor-update.md +++ b/text/0000-native-class-constructor-update.md @@ -68,12 +68,18 @@ and the `create` method to circumvent this issue. This change _would_ be a breaking change to the behavior of native classes today, and a change from the previous class RFC. This will impact early adopters -and should be made with that in mind. However, because native classes never -officially shipped as part of Ember's public API (an announcement was not made, -docs have not been written, etc), this RFC proposes that the change would _not_ -be considered a breaking change _for the purposes of semver_. This would allow -us to ship the change during the Ember v3 release cycle, and prevent more code -from being built on top of the previous behavior. +and should be made with that in mind. It would _not_ be a change that breaks the +behavior of the community solutions to class fields mentioned above, and all +other changes would be relatively easy to create a safe codemod for (essentially +converting `constructor` -> `init` in affected classes), so the impact _should_ +be minimal. + +Because native classes never officially shipped as part of Ember's public API +(an announcement was not made, docs have not been written, etc), this RFC +proposes that the change would _not_ be considered a breaking change _for the +purposes of semver_. This would allow us to ship the change during the Ember v3 +release cycle, and prevent more code from being built on top of the previous +behavior. ## Detailed design @@ -141,7 +147,11 @@ assigned each time a class is initialized, but these are easier to work around. ### Injections and the `init` hook One side effect of this change is that injections will not be available on the -class instance during the `constructor` phase. The ideal behavior here is +class instance during the `constructor` phase. This behavior is not very +commonly used - based on an informal community survey we found only a few usages +\- but it _does_ exist and have its use cases. + +Figuring out the ideal behavior of injections during the constructor phase is outside of the scope of this RFC, and is something that should be discussed in future RFCs. For the time being, users can still rely on the `init` hook, which will continue to be called after all injections and properties have been @@ -202,13 +212,11 @@ demonstrate the behaviors of classes throughout the guides and API docs. One thing we should make clear is that EmberObject will likely be deprecated in the near future, and that ideally for non-Ember classes (things that aren't Components, Services, etc.) users should drop EmberObject altogether and use -native classes only. For the purposes of codemodding, however, this will not be -possible, since EmberObject based classes still have many differences in common -usage from standard native classes. +native classes only. ## Drawbacks -This would be a breaking change that would negatively affect early adopters. +This would be a breaking change that could negatively affect early adopters. ## Alternatives From def613da0b4f511d737cdbecccdc3749c861243b Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Fri, 15 Jun 2018 11:34:09 -0700 Subject: [PATCH 3/5] add explicit deprecation timeline --- text/0000-native-class-constructor-update.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/text/0000-native-class-constructor-update.md b/text/0000-native-class-constructor-update.md index bc1764b3f2..6f10fe6978 100644 --- a/text/0000-native-class-constructor-update.md +++ b/text/0000-native-class-constructor-update.md @@ -162,6 +162,9 @@ assigned to the instance. It was previously possible to use `new` syntax with EmberObject. While this was not considered public API, it has technically worked and been under test since the early days of Ember, and may fall under the category of intimate API. +Ideally, we would deprecate this usage as a private/intimate API, which would +mean supporting it through the next LTS version, and dropping support after +(currently, this would mean dropping it at `v3.5.0`). We can continue to support this behavior in a backwards compatible way while deprecating it with one final tweak to the change above: @@ -182,7 +185,11 @@ class EmberObject { return this; } - deprecate('using new with EmberObject has been deprecated', true); + deprecate('using `new` with EmberObject has been deprecated. Pleas use `create` instead.', false, { + id: 'object.new-constructor', + until: '3.5.0' + }); + initialize(this, props); } From 7f8850f01ddcc75d08903af6d00c7d560a948362 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Fri, 15 Jun 2018 12:27:10 -0700 Subject: [PATCH 4/5] update example to not use this --- text/0000-native-class-constructor-update.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-native-class-constructor-update.md b/text/0000-native-class-constructor-update.md index 6f10fe6978..d9e77285e6 100644 --- a/text/0000-native-class-constructor-update.md +++ b/text/0000-native-class-constructor-update.md @@ -118,8 +118,8 @@ class EmberObject { static create(props) { let instance = new this(props); - Object.assign(this, props); - this.init(); + Object.assign(instance, props); + instance.init(); return instance; } From c7d602d8de3e91172dc3395e5bc40a3e30d2147f Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Sun, 8 Jul 2018 13:20:32 -0700 Subject: [PATCH 5/5] incorporate feedback about alternatives --- text/0000-native-class-constructor-update.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/text/0000-native-class-constructor-update.md b/text/0000-native-class-constructor-update.md index d9e77285e6..e44ea996c4 100644 --- a/text/0000-native-class-constructor-update.md +++ b/text/0000-native-class-constructor-update.md @@ -185,7 +185,7 @@ class EmberObject { return this; } - deprecate('using `new` with EmberObject has been deprecated. Pleas use `create` instead.', false, { + deprecate('using `new` with EmberObject has been deprecated. Please use `create` instead.', false, { id: 'object.new-constructor', until: '3.5.0' }); @@ -227,9 +227,21 @@ This would be a breaking change that could negatively affect early adopters. ## Alternatives -We could leave the behavior as is, and choose a method for defaulting to +* We could leave the behavior as is, and choose a method for defaulting to standardize on. +* We could make this change behind a feature flag and require users to opt-in +to the new behavior, like optional features that currently exist. This would +have to be a build time feature flag, since the area is very performance +sensitive. Given native classes are not yet public API, if we were to do this we +should probably still default to enabling the new behavior and recommending it +as the preferred path. + +* We could not deprecate `new EmberObject` altogether, and instead only +deprecate passing properties to the constructor. While this would work as a +temporary solution, it may also encourage users to continue using EmberObject +instead of switching to native classes, which is ultimately the long term goal. + ## Unresolved questions How do we handle DI during the construction phase?