serverless: Configure all promise returning functions as async functions

Use case description

Paves path to https://github.com/serverless/serverless/issues/7171

Ideally we should use just native promises and async/await syntax. Still refactor of whole code seems as too big and problematic task for single PR, it’s better to divide it into smaller clear tasks.

As a first distinct step I believe it’ll simply be good to configure all promise returning functions as async functions (with no adjustments to its bodies, the internal promise flow should remain configured as-is)

Proposed solution

Search for all functions which return an outcome of any BbPromise* (bluebird), Promise* or x.then() operation, and (1) confirm weather it’s justified that function returns promise. if there are no async calls (with exception to fs sync variants which we should refactor in next step to async) (2a) Reconfigure function so it doesn’t return promises. If it’s otherwise (2b) add async keyword in front of function keyword (that’s all, do not adjust function body content)

To avoid making this PR too problematic to review, nothing more should be covered in PR.

It’s likely this PR will already expose some issues, as Framework has logic parts where promise returning functions are in some scenarios treated as sync and in other as async. All handling like that should be fixed with this PR.

As project covers a lot of files, let’s split this work into multiple PR’s each covering following areas:

About this issue

  • Original URL
  • State: closed
  • Created 4 years ago
  • Reactions: 1
  • Comments: 34 (23 by maintainers)

Commits related to this issue

Most upvoted comments

can cross out test/unit/lib/plugins/interactiveCli since there is no async tests under it

+1
sleepwithcoffee on Mar 18, 2023

@nyamba Yes, looks like it’s the case indeed - we had some extensive refactoring implemented recently for that module so it might’ve changed with that. I’m going to cross it out from the list

+1
pgrzesik on Jun 25, 2021

Hello @nyamba - totally, we’d be happy to accept PRs for that. The list in the issue should be up-to-date - you can pick one from the list that you like and work on it 👍

+1
pgrzesik on Jun 25, 2021

@KishorBudhathoki10 great thanks for reaching out to help! Still this one is already being covered by @JeremyScott-IO (note the assignment). Please search other issues with good first issue label (which have no one assigned), and see if you can help with those (?)

Concerning code attachments, please use compare links (e.g. you may create a branch in your fork and send us link to its compare view). Sharing zip archives doesn’t feel safe

+1
medikoo on Oct 19, 2020

I’ve noticed a lot of methods that return promises aren’t async, and some ()=> module.exports are also returning Promises. Should I be converting methods and these module.exports to async as well?

Ideally functions/methods which do not involve any async operations (but e.g. simply return BbPromise.resolve(..)) should be configured as sync (so have return BbPromise.resolve(x) replaced with return x and return BbPromise.reject(e) replaced with with throw e). Still you may approach situations that making them sync breaks things in a way that more sophisticated refactor is needed, keep such cases async to not overcomplicate this PR (let’s add async keyword and TODO comment explaining future intent of having them sync)

+1
medikoo on Oct 12, 2020

Hey, I’d like to take this on!

Just to make sure I understand, don’t modify the functions, aside from changing the method to async. However if after changing these I find problems where calling functions can’t handle the now async method, I should fix these calling functions in this same PR?

+1
It-Is-Jeremy on Oct 8, 2020
Read more comments on GitHub
← serverless: 'Concurrent update operation' error for multi-function service results in both deployment and rollback failure.
serverless: [Critical Bug] `sls deploy` does not create API endpoints in `Stages` section of Amazon API Gateway →