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

Replaced Promise.join() with .all() in user model #14972

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

Emmanuel-Melon
Copy link
Contributor

Got some code for us? Awesome 🎊!

Please include a description of your change & check your PR against this list, thanks!

  • There's a clear use-case for this code change, explained below
  • Commit message has a short title & references relevant issues
  • The build will pass (run yarn test:all and yarn lint)

We appreciate your contribution!

Also, if you'd be interested in writing code like this for us more regularly, we're hiring:
https://careers.ghost.org/product-engineer-node-js

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #14972 (47f5758) into main (3231863) will increase coverage by 8.50%.
The diff coverage is 60.00%.

❗ Current head 47f5758 differs from pull request most recent head b76ffc8. Consider uploading reports for the commit b76ffc8 to get more accurate results

@@            Coverage Diff             @@
##             main   #14972      +/-   ##
==========================================
+ Coverage   52.60%   61.10%   +8.50%     
==========================================
  Files        1392      596     -796     
  Lines       89130    47668   -41462     
  Branches    10131     4293    -5838     
==========================================
- Hits        46884    29127   -17757     
+ Misses      42194    18493   -23701     
+ Partials       52       48       -4     
Impacted Files Coverage Δ
core/server/models/user.js 49.03% <60.00%> (ø)
...ore/core/server/services/members/exporter/query.js
ghost/core/core/server/lib/mobiledoc.js
ghost/core/core/frontend/helpers/excerpt.js
...core/core/frontend/services/rendering/templates.js
...t/core/core/server/services/posts/posts-service.js
ghost/core/core/server/data/db/state-manager.js
...r/api/endpoints/utils/serializers/input/members.js
ghost/admin/app/components/modal-unsuspend-user.js
ghost/core/core/frontend/web/middleware/index.js
... and 1979 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@markstos
Copy link
Contributor

Why is this change considered an improvement? The docs for Promise.join say "While .all is good for handling a dynamically sized list of uniform promises, Promise.join is much easier (and more performant) to use when you have a fixed amount of discrete promises that you want to coordinate concurrently."

Did you check to see which method performed better?

If the the goal was to use a native Promise method instead of a Bluebird method, Bluebird is still required after this change is made, so there's not much benefit there.

@vikaspotluri123
Copy link
Member

@markstos see #14882 🙂

@Emmanuel-Melon Emmanuel-Melon marked this pull request as ready for review July 15, 2022 10:59
@ErisDS ErisDS changed the title feat: replace Promise.join with promise.all() Replaced Promise.join() with .all() in user model Aug 24, 2022
@ErisDS ErisDS merged commit d9f0db6 into TryGhost:main Aug 24, 2022
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.

4 participants