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

feat(content): add async await js article #109

Merged
merged 1 commit into from
Oct 3, 2019
Merged

feat(content): add async await js article #109

merged 1 commit into from
Oct 3, 2019

Conversation

ddivad195
Copy link
Member

feat(content): add Async / Await JS article

Add a basic article in the Promises section of Javascript on Async / Await.

There are possibly scenarios I have missed or not properly explained here, as it is my first contribution to the site. If you need me to make any changes feel free to suggest.

Added:

  • Async / Await article for JS resources section.

Referenced in #87

src/content/resources/javascript/promises/async-await.md Outdated Show resolved Hide resolved
Comment on lines +88 to +93
(async () => {
let data = await foo();
console.log(data); // "foo"
})();
Copy link
Member

Choose a reason for hiding this comment

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

Might be a good idea to to add a single link to the IIFE you're using here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean here? Do you mean add a link to what an IIFE is?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, either that or just turn it into a regular function call. I think we probably want to keep the examples as simple as possible in every way

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I get what you're saying, but to me an IIFE here makes sense as people can copy / paste the code and it will just work. If it was a normal function instead, they would have to make an async function themselves if they wanted to run the code and check the output, which was the reason I used it here. I'll see if I can find a good link to put in here maybe that can explain them, or maybe even write an explanation in some other article here for how they work.

}
```

## Beginner Mistakes
Copy link
Member

Choose a reason for hiding this comment

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

This section should absolutely be mentioning the mistake of wrapping promises inside new Promise, that's something I've seen too many times.

Copy link
Member

Choose a reason for hiding this comment

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

Although we might just want to edit the original promise article for that instead, I guess I could see that going either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - I guess adding in to the original promise article might be better here, as I have that article listed in the recommended reading section, and can add that one there explicitly.

People should read that one first, so the way I was approaching this section here was like an addition to the common mistakes from the original promise article, rather than adding them in both places.

created_at: "2019/10/01"
title: Async Await
recommended_reading:
- javascript/promises
Copy link
Collaborator

Choose a reason for hiding this comment

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

this links to a 404 (at least for now), can we link to javascript/promises/intro?

Copy link
Member Author

Choose a reason for hiding this comment

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

cool, fixed that now, thanks.

@veksen
Copy link
Collaborator

veksen commented Oct 2, 2019

By the way, ignore the codacy check, we are not using it atm and it's improperly set up (ping @Xetera).

Could you please rebase against latest master, and squash your 3 commits?

@ddivad195
Copy link
Member Author

@veksen will do.

I have just rebased against master and squashed the commits. Let me know if I need to do anything else.

@veksen
Copy link
Collaborator

veksen commented Oct 3, 2019

TY! It's good for me, will merge in a bit. Little advice, if you can create a new branch when you make PRs on your fork, it makes it a lot easier if anybody wants to use your forked branch locally (to make edits, or simply checkout).

Ty for the contribution!

@veksen
Copy link
Collaborator

veksen commented Oct 3, 2019

@Xetera merging this, if we want further changes, let's introduce a new PR

@veksen veksen merged commit 7d1cbb1 into the-programmers-hangout:master Oct 3, 2019
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.

3 participants