Replaces `this.get with ES5 getter where possible (#2303) in v3.2.0 #87

cspanring · 2018-06-10T17:08:49Z

Attempt to check off task 1 in emberjs/guides#2303

@jenweber please have a look and/or pull in more eyes to make sure the changes are OK.

jenweber · 2018-07-09T02:42:28Z

guides/v3.2.0/components/the-component-lifecycle.md

@@ -66,7 +66,7 @@ import Component from '@ember/component';
 export default Component.extend({
  init() {
    this._super(...arguments);
-    this.errors = [];
+    this.set('errors', []);


This is a really good catch, thank you

seconded 🎉

jenweber

I think this looks good. I would like 1 more reviewer on this before we merge. The diff also needs to be applied to 3.3 after it's merged.

mansona

Ok so this PR is amazing 😍 I was mostly scrolling down saying to myself "awesome... awesome!!! AWESEOME!!" 😂

My comments are in 3 categories mostly:

tiny typos (I think there are only 2 of them)
things that I don't fully understand
things that have subtly changed the behaviour of the "defensiveness" of the code

This last point can be ignored if we don't think it's important/matters. Essentially in the "bad old days" doing something like this.get('something.you.want') would not throw an error when something or you were null or undefined. I like to think of it as chain accessor protection.

Nowadays we will be seeing more crashes in JS because we're removing this automatic protection but the flip side is that we're getting closer to normal JavaScript behaviour 😂

Anyway, the only points that need to be fixed in this PR are the typos, everything else is opinion 👍

mansona · 2018-07-09T07:52:09Z

guides/v3.2.0/applications/dependency-injection.md

@@ -245,13 +245,13 @@ import { getOwner } from '@ember/application';
 export default Component.extend({
  audioService: computed('song.audioType', function() {
    let applicationInstance = getOwner(this);
-    let audioType = this.get('song.audioType');
+    let audioType = this.song.audioType;


This is probably just a style thing but if you forget to pass song into play-audio it will throw an Exception here, whereas the old version would just return undefined or null or something 😂

I don't know what the best way to communicate this would be 🤔 I personally still use get() in cases like this but I import it from import { get } from '@ember/object' and use it in the form: get(this, 'song.audioType')

Thoughts?

I can see your point about the safeguard, and I'm often doing the same in Emberland. However, adopting more of the "newer" dot-notation syntax for getters, I would probably add an early return if (!this.song) return null; in this particular case, since the core of this computed property requires a song.

mansona · 2018-07-09T07:55:48Z

guides/v3.2.0/components/handling-events.md

@@ -69,7 +69,7 @@ export default Component.extend({

  drop(event) {
    let id = event.dataTransfer.getData('text/data');
-    this.get('action')(id);
+    this.action(id);


is this update potentially hiding the fact that action is now essentially just a variable name? I wonder if because it's a "thing" in Ember it might be worth updating it to be called dropAction() instead?

This is potentially a super minor point so ignore if it's not important 😂

I'd second this. I could see myself getting confused by an action called "action"

agree, will do.

mansona · 2018-07-09T07:58:04Z

guides/v3.2.0/components/the-component-lifecycle.md

@@ -66,7 +66,7 @@ import Component from '@ember/component';
 export default Component.extend({
  init() {
    this._super(...arguments);
-    this.errors = [];
+    this.set('errors', []);


seconded 🎉

mansona · 2018-07-09T08:01:17Z

guides/v3.2.0/models/relationships.md

@@ -190,7 +190,7 @@ export default PaymentMethod.extend({
  linkedEmail: DS.attr(),

  obfuscatedIdentifier: computed('linkedEmail', function () {
-    let last5 = this.get('linkedEmail').split('').reverse().slice(0, 5).reverse().join('');
+    let last5 = this.linkedEmail.split('').reverse().slice(0, 5).reverse().join('');


does this work with Models? I have in the back of my mind that you still need to use this.get() for some things and I have a feeling that it has something to do with Ember Data but I don't know the specifics... Can anyone confirm?

Will verify. ES5 getters can't be used for anything that implements a Proxy and Ember Data is using them a lot.

Verified that ES5 getters work on Ember Data Model attributes (as long as they are not relations). If it hasn't, this should probably be pointed out somewhere in the guides.

mansona · 2018-07-09T08:01:46Z

guides/v3.2.0/object-model/classes-and-instances.md

@@ -117,7 +117,7 @@ import EmberObject from '@ember/object';

 const Person = EmberObject.extend({
  helloWorld() {
-    alert(`Hi, my name is ${this.get('name')}`);
+    alert(`Hi, my name is ${tthis.name}`);


double t in tthis

good catch!

mansona · 2018-07-09T08:02:01Z

guides/v3.2.0/object-model/classes-and-instances.md

@@ -152,7 +152,7 @@ import EmberObject from '@ember/object';

 const Person = EmberObject.extend({
  init() {
-    alert(`${this.get('name')}, reporting for duty!`);
+    alert(`${tthis.name}, reporting for duty!`);


CodingItWrong

Looks great! Most of my comments are about some additional small tweaks for simplification.

CodingItWrong · 2018-07-09T10:24:41Z

guides/v3.2.0/components/the-component-lifecycle.md

-    this.set('items', this.get('items').map((item) => {
-      if (item.id === this.get('selectedItem.id')) {
+    this.set('items', this.items.map((item) => {
+      if (item.id === this.selectedItem.id) {


Unrelated to this PR, but it seems like this map() is mutating the items. I think some kind of for…of or forEach could better express that we are changing the items in place. But no need to make the change in this PR unless you feel especially motivated to do so :-)

I can look at it and propose a better example, no problem.

CodingItWrong · 2018-07-09T10:26:13Z

guides/v3.2.0/components/triggering-changes-with-actions.md

@@ -186,7 +186,7 @@ export default Component.extend({
 });
 ```

-`this.get('onConfirm')` will return the function passed from the parent as the
+`this.onConfirm` will return the function passed from the parent as the
 value of `onConfirm`, and the following `()` will invoke the function.


I wonder if we still need the explanation about "the following ()". When "get()" was needed I did feel like I needed the explanation of the extra parens, but now it's just invoking a function property as usual.

yep, agree, can tweak or remove that phrase.

CodingItWrong · 2018-07-09T10:29:36Z

guides/v3.2.0/object-model/computed-properties-and-aggregate-data.md

@@ -25,7 +25,7 @@ export default Component.extend({
  },

  titles: computed('todos.[]', function() {
-    let todos = this.get('todos');
+    let todos = this.todos;


Could probably just inline this.todos in the following line, unless you think there's extra clarity separating it out.

CodingItWrong · 2018-07-09T10:31:02Z

guides/v3.2.0/object-model/computed-properties.md

-    let firstName = this.get('firstName');
-    let lastName = this.get('lastName');
+    let firstName = this.firstName;
+    let lastName = this.lastName;


I'd probably in-line these into the template string instead of assigning to local variables

CodingItWrong · 2018-07-09T10:31:22Z

guides/v3.2.0/object-model/computed-properties.md

-    let firstName = this.get('firstName');
-    let lastName = this.get('lastName');
+    let firstName = this.firstName;
+    let lastName = this.lastName;


Same as previous

CodingItWrong · 2018-07-09T10:32:57Z

guides/v3.2.0/testing/testing-components.md

@@ -15,7 +15,7 @@ export default Component.extend({
  attributeBindings: ['style'],

  style: computed('name', function() {
-    const name = this.get('name');
+    const name = this.name;


Same as above

CodingItWrong · 2018-07-09T10:35:16Z

guides/v3.2.0/tutorial/autocomplete-component.md

-        return this.get('store').query('rental', { city: param });
-        return this.get('store')
+        return this.store.query('rental', { city: param });
+        return this.store


It might just be the formatting here on my phone, but I'm not even sure I understand what's happening in these lines. Is it two return statements in a row? The second one would never be reached, right? If so I think it'd be good to quickly clean it up as part of this PR.

good catch, this needs to be fixed, will do.

this is displayed as diff in the rendered version: https://guides.emberjs.com/release/tutorial/autocomplete-component/#toc_handling-results-coming-back-at-different-times
the redundant code makes sense here.

CodingItWrong · 2018-07-09T10:35:46Z

guides/v3.2.0/tutorial/autocomplete-component.md

-        return this.get('store').findAll('rental');
-        return this.get('store')
+        return this.store.findAll('rental');
+        return this.store


Same as above

same, displayed as diff in the rendered guides.

CodingItWrong · 2018-07-09T10:36:45Z

guides/v3.2.0/tutorial/service.md

@@ -196,14 +196,14 @@ export default Component.extend({

  didInsertElement() {
    this._super(...arguments);
-    let location = this.get('location');
-    let mapElement = this.get('maps').getMapElement(location);
+    let location = this.location;


this.location can probably be inlined in the following line

cspanring · 2018-07-18T13:29:40Z

@jenweber @mansona @CodingItWrong I think I addressed all your feedback. Please let me know if there's anything else. Otherwise we could merge and I could apply the diff to 3.3.

apply cspanring's getter work to 3.3 #87

jenweber reviewed Jul 9, 2018

View reviewed changes

jenweber approved these changes Jul 9, 2018

View reviewed changes

jenweber added the needs a reviewer label Jul 9, 2018

mansona reviewed Jul 9, 2018

View reviewed changes

CodingItWrong reviewed Jul 9, 2018

View reviewed changes

cspanring force-pushed the es5-getters-2303 branch from b28bf1d to e6a0737 Compare July 18, 2018 13:24

replaces this.get pattern with ES5 getter where possible (#2303)

71f0f4d

cspanring force-pushed the es5-getters-2303 branch from e6a0737 to 71f0f4d Compare July 18, 2018 13:27

jenweber merged commit 6364485 into ember-learn:master Jul 29, 2018

jenweber mentioned this pull request Jul 29, 2018

apply cspanring's getter work to 3.3 #87 #122

Merged

acorncom added a commit that referenced this pull request Jul 30, 2018

Merge pull request #122 from jenweber/es6-getters

218cca2

apply cspanring's getter work to 3.3 #87

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaces `this.get with ES5 getter where possible (#2303) in v3.2.0 #87

Replaces `this.get with ES5 getter where possible (#2303) in v3.2.0 #87

cspanring commented Jun 10, 2018

jenweber Jul 9, 2018

mansona Jul 9, 2018

jenweber left a comment

mansona left a comment

mansona Jul 9, 2018

mansona Jul 9, 2018

cspanring Jul 16, 2018

mansona Jul 9, 2018

CodingItWrong Jul 9, 2018

cspanring Jul 16, 2018

mansona Jul 9, 2018

mansona Jul 9, 2018

cspanring Jul 16, 2018

cspanring Jul 18, 2018

mansona Jul 9, 2018

cspanring Jul 16, 2018

mansona Jul 9, 2018

CodingItWrong left a comment

CodingItWrong Jul 9, 2018

cspanring Jul 16, 2018

CodingItWrong Jul 9, 2018

cspanring Jul 16, 2018

CodingItWrong Jul 9, 2018

CodingItWrong Jul 9, 2018

CodingItWrong Jul 9, 2018

CodingItWrong Jul 9, 2018

CodingItWrong Jul 9, 2018

cspanring Jul 16, 2018

cspanring Jul 18, 2018

CodingItWrong Jul 9, 2018

cspanring Jul 18, 2018

CodingItWrong Jul 9, 2018

cspanring commented Jul 18, 2018

Replaces `this.get with ES5 getter where possible (#2303) in v3.2.0 #87

Replaces `this.get with ES5 getter where possible (#2303) in v3.2.0 #87

Conversation

cspanring commented Jun 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenweber left a comment

Choose a reason for hiding this comment

mansona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodingItWrong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspanring commented Jul 18, 2018