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

fix(lib): typo in error message #68

Merged
merged 7 commits into from
Apr 27, 2020
Merged

fix(lib): typo in error message #68

merged 7 commits into from
Apr 27, 2020

Conversation

Kerumen
Copy link
Contributor

@Kerumen Kerumen commented Apr 24, 2020

The PR is now only a fix for a typo problem.


It's useless to add an await when we already return a promise.

See the eslint rule for example: https://eslint.org/docs/rules/no-return-await

(also fixed a small typo)

Edit: initially I removed both 2 awaits but as the first one is required to check the error, I created a variable to remove the return await.

Tell me what do you think :)

@Kerumen Kerumen changed the title Remove return await fix(lib): remove return await Apr 24, 2020
@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #68 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #68   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           37        37           
  Branches        12        12           
=========================================
  Hits            37        37           
Impacted Files Coverage Δ
lib/index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cacf01...78d9b1f. Read the comment docs.

@vvo
Copy link
Owner

vvo commented Apr 27, 2020

Hey there thanks for the PR, I was curious so I did my research too on this subject. It seems that the world of JavaScript developers is not quite sure about whether or not return await is an issue in term of performance and debugging information.

See:

To be honest, unless you find a bug or performance issue caused by the lines you updated, I won't be merging this.

I still welcome your changes on the typos on the errors though :)

Also, pure stylistic but I like to know whether or not a method is an async one. If you remove await from await ironStore then you now need to know that ironStore is an async function.

Thanks anyway and I hope you'll submit more PRs here!

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
@vvo
Copy link
Owner

vvo commented Apr 27, 2020

The whole "no-return-await" rule/discussion seems to boil down to:

  • "This is incorrect code!" (from no-return-await peeps)
  • "This is less performant" (from no-return-await peeps)
  • "This is less clear" (from both side)

To confirm, unless we can prove it's significantly slower in real-world conditions (not in a loop trying to produce a significant slowdown). This is a stylistic choice and one that for now I haven't jumped on so we'll keep this part as it is, thanks!

@Kerumen
Copy link
Contributor Author

Kerumen commented Apr 27, 2020

Thanks for this overview on this topic, I was "blindly" following my eslint rule.

Sorry for the useless PR, I'll accept your changes to commit only the typo fix.

Co-Authored-By: Vincent Voyer <vincent@codeagain.com>
@vercel vercel bot temporarily deployed to Preview April 27, 2020 11:57 Inactive
Co-Authored-By: Vincent Voyer <vincent@codeagain.com>
Co-Authored-By: Vincent Voyer <vincent@codeagain.com>
@Kerumen Kerumen changed the title fix(lib): remove return await fix(lib): typo in error message Apr 27, 2020
@vvo
Copy link
Owner

vvo commented Apr 27, 2020

🎉 This PR is included in version 3.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@vvo vvo added the released label Apr 27, 2020
@Kerumen Kerumen deleted the patch-1 branch April 28, 2020 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants