Skip to content
This repository has been archived by the owner on May 26, 2019. It is now read-only.

Draft for promises #2317

Merged

Conversation

karan-pathak
Copy link
Contributor

@karan-pathak karan-pathak commented Apr 3, 2018

@jenweber
Copy link
Contributor

Hi @karan-pathak ! This is awesome 😄 I see a few typos and small grammar things - is it ok if I add a commit to your PR, or would you rather that I send them to you and you can make the changes?

```javascript
store.findRecord('person', 1).then(function(person) {

store.findRecord('post', 1) //query for another record.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding correctly this example, shound't this line have return so the next then would receive the returned value, in this case post?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josemarluedke yes, you are correct. Thanks for the observation. I've made changes to the example, and added some explanation, can you review it again?

@karan-pathak
Copy link
Contributor Author

Hi @jenweber, Thanks for reviewing it! please feel free to make changes to this PR, and if you want me to make the changes then just let me know :)

@jenweber
Copy link
Contributor

@karan-pathak take a look and let me know what you think of my edits!

After looking at this more closely, I think it needs 2 more points:

  • a sentence or two about how variables modified inside a promise will be undefined for non-Promise-wrapped code (I don't know how to say this better. Does that make any sense to you?)
  • A closing statement

I am also going to ping @locks and/or @jaredgalanis for review. Promises are hard so some more feedback would be good.

@karan-pathak
Copy link
Contributor Author

@jenweber, great edits! The extra points that you suggested seem valid. I'll add them to this PR.

Also, more feedback is always welcome :)

@toddjordan
Copy link
Contributor

Thanks for contributing @karan-pathak !

@toddjordan toddjordan merged commit d2d1782 into emberjs:master Apr 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants