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

Refactor all BbPromise.x usage which doesn't have native counterparts #8369

Open
6 of 12 tasks
medikoo opened this issue Oct 8, 2020 · 8 comments · Fixed by #11797
Open
6 of 12 tasks

Refactor all BbPromise.x usage which doesn't have native counterparts #8369

medikoo opened this issue Oct 8, 2020 · 8 comments · Fixed by #11797

Comments

@medikoo
Copy link
Contributor

medikoo commented Oct 8, 2020

Use case description

Partially covers #7171

Should be addressed after #8368

Bluebird exposes a lot of functions and methods that have no standard counterparts, we should refactor it to native promises syntax

Proposed solution

Refactor to async/await all functions that contain usage of following methods:

Note: To avoid large, problematic to handle PR's. let's replace each function with different PR

  • BbPromise.bind - Most likely, in all used cases, bind wrapper can be just removed (no counterpart needed)
  • BbPromise.delay - Timeout could be implemented with help of timers-ext/promise/sleep
  • BbPromise.each - We can simply use for (..) { await .. } construct
  • BbPromise.fromCallback - Use case for which it's used, it's probably no longer actual (if it is, we need to come up with new Promise(..) construct)
  • BbPromise.join - Replace with Promise.all
  • BbPromise.map - Replace with Promise.all
  • BbPromise.mapSeries - We can simply use for (..) { await .. } construct
  • BbPromise.promisify - When fs functions are wrapped, resort to fs.promises. In other cases use require('util').promisify
  • BbPromise.promisifyAll - When fs functions are wrapped, resort to fs.promises. In other cases use require('util').promisify
  • BbPromise.props - Refactor with Promise.all
  • BbPromise.reduce - We can simply use for (..) { await .. } construct
  • BbPromise.try - As all functions containing it are now declared as async, this wrapper can simply be just removed
@ifitzsimmons
Copy link
Contributor

ifitzsimmons commented Jan 2, 2021

I created a branch to start chipping away at this. I refactored everything under lib/plugins/aws/deploy. Link to branch is here.

I can submit the pull request once my PR for #8368 has been merged. I used the async tagging changes from that branch while refactoring. I'll refactor some of the other folders tomorrow.

@medikoo - let me know if my work is a little premature. I actually have a good amount of experience refactoring Promises and BBPromises to async/await, so this stuff shouldn't take me too too long. Hope this stuff helps

@medikoo
Copy link
Contributor Author

medikoo commented Jan 4, 2021

@ifitzsimmons great thanks for help! Ideally if we address it step by step, as outlined in issues. First by addressing #8368, and we have a lot to cover over there. Have you checked that issue?

@ifitzsimmons
Copy link
Contributor

@medikoo I did. I made the async tagging updates for #8368 and am just waiting for it to be merged. Do you want to complete ALL of #8368 before refactoring the Promise code, or can we start refactoring the Promises to async/await for a folder once we tag the Promise-based functions with async and merge it in?

@medikoo
Copy link
Contributor Author

medikoo commented Jan 10, 2021

Do you want to complete ALL of #8368

Yes, ideally if that step is finalized entirely before moving with next steps

@sleepwithcoffee
Copy link
Contributor

Create #11797 to refactor BbPromise.each()
I don't see these being use:
BbPromise.delay
BbPromise.fromCallback
BbPromise.join
BbPromise.promisify
BbPromise.props
BbPromise.reduce

So we can safely cross them out in the original comment

@medikoo
Copy link
Contributor Author

medikoo commented Mar 13, 2023

@sleepwithcoffee thanks, I've updated the description. Concerning this scope of work, ideally if first we cover #8368. Intentionally this async refactor, was planned into multiple carefully ordered steps - so we do not introduce some regressions (not all of the affected parts are well tested)

@sleepwithcoffee
Copy link
Contributor

hi @medikoo I think this was closed by accident, let's reopen this ticket

@medikoo medikoo reopened this Mar 20, 2023
@medikoo
Copy link
Contributor Author

medikoo commented Mar 20, 2023

@sleepwithcoffee thanks for heads up. Just a note. Ideally if we cover #8368 entirely first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants