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.

Sign up for GitHub

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

Jump to bottom

Configure all promise returning functions as async functions #8368

Closed
43 tasks done
medikoo opened this issue Oct 8, 2020 · 34 comments · Fixed by #11798, #11799 or #11803
Closed
43 tasks done

Configure all promise returning functions as async functions #8368

medikoo opened this issue Oct 8, 2020 · 34 comments · Fixed by #11798, #11799 or #11803
Labels
cat/dependency good first issue help wanted refactor

Comments

@medikoo
Copy link
Contributor

medikoo commented Oct 8, 2020 •
edited

Use case description

Paves path to #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:
This was referenced Oct 8, 2020
Open
Open
@It-Is-Jeremy
Copy link

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?

@medikoo
Copy link
Contributor Author

medikoo commented Oct 8, 2020

@JeremyScott-IO that'll be great go for it! And yes your understanding is perfectly correct

It-Is-Jeremy pushed a commit to It-Is-Jeremy/serverless that referenced this issue Oct 8, 2020
[WIP] Add async to obvious functions w/ promises
a9a24fd 
Initial adding of async. NPM test all still passing.
Looking to cleanup and handle async/change calling
functions if required

resolves:serverless#8368
@Maxgit3
Copy link

Maxgit3 commented Oct 9, 2020

If you need any more help, I would be willing to chip in.

@It-Is-Jeremy
Copy link

Hi @medikoo,

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?

@medikoo
Copy link
Contributor Author

medikoo commented Oct 12, 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)

@medikoo
Copy link
Contributor Author

medikoo commented Oct 16, 2020

@JeremyScott-IO if it appears as too large task for one PR, we may splt it. so e.g. with one PR we cover only designated folders. Let me know, I'll be more than happy to outline such roadmap if needed. Then more of us can help covering it.

Great thanks again, for looking into it!

@It-Is-Jeremy
Copy link

@medikoo, I'm going to have a fair amount of time across the course of this week. I can give it a go, but I'm totally fine if you guys want to split it out and allow other contributors :)

Let me know!

@medikoo
Copy link
Contributor Author

medikoo commented Oct 19, 2020

@JeremyScott-IO Let's see how it goes then :) If it'll appear challenging we'll propose to seclude some areas to other PR's

@KishorBudhathoki10
Copy link

Hello Everyone. I am new to working with open source projects. I want to contribute to this project and this issue seems the right place for me to get started. I have questions regarding the folder structure. Should I also make an update inside the plugins folder? I have refactored some code inside ./lib/plugins/aws/invokeLocal/index.js to use async/await which I have attached below. Should I push it for check? Also which are the main folders that should be refactored?

index.js.zip

Thank you in Advance.

@medikoo
Copy link
Contributor Author

medikoo commented Oct 19, 2020

@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

It-Is-Jeremy pushed a commit to It-Is-Jeremy/serverless that referenced this issue Oct 26, 2020
Convert eligible functions to async functions
ca37283 
Converted functions to async, confirmed no breakage by running npm test.
Added todo notes for some functions

Resolves:serverless#8368
@Edu93Jer
Copy link

Hi guys, I am new at open source and would like to contribute to the project, this issue looks find to get started, but please guys could you please let me know how is it going? I saw @JeremyScott-IO has started, but any update, can I help with something, how should I start working on it, please could you explain to me a little more @medikoo ...

@medikoo
Copy link
Contributor Author

medikoo commented Dec 28, 2020

@Edu93Jer it appears it got stuck, and it's not surprising as this covers quite a lot.

To make it easier I've split the work into multiple areas. Please see updated description. With that approach we should be able to move it effectively forward.

PR's welcome!

@Edu93Jer Edu93Jer mentioned this issue Dec 29, 2020
Closed
@ifitzsimmons
Copy link
Contributor

@Edu93Jer which part of this issue did you tackle. I have some time over the next few days and was going to clean some of this up but I don't want to step on any toes.

@Edu93Jer
Copy link

Hi @ifitzsimmons I took lib and lib/clases go ahaed and take another files, actually I made a mistake on my PR, you can check it, for not to do the same hehehe, I will correct my PR, but go ahead with the other files if you want.
Cheers, and a Happy New Year!

@ifitzsimmons ifitzsimmons mentioned this issue Jan 1, 2021
Merged
This was referenced Feb 1, 2021
Closed
Merged
Merged
Merged
This was referenced Mar 11, 2023
Merged
Merged
Closed
Merged
Merged
Merged
@medikoo medikoo reopened this Mar 13, 2023
@medikoo medikoo reopened this Mar 13, 2023
This was referenced Mar 14, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@sleepwithcoffee
Copy link
Contributor

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

This was referenced Mar 19, 2023
Merged
Merged
Merged
Merged
@sleepwithcoffee
Copy link
Contributor

sleepwithcoffee commented Mar 21, 2023 •
edited

hi @medikoo can cross out test/unit/lib/plugins/create and test/unit/lib/plugins as well, files directly under test & test/unit/lib.

lib/plugins/plugin & lib/plugins/aws/remove are also clear

@sleepwithcoffee sleepwithcoffee mentioned this issue Mar 22, 2023
Merged
@sleepwithcoffee
Copy link
Contributor

hi @medikoo I believe this ticket can be closed since all folders are corrected

@medikoo
Copy link
Contributor Author

medikoo commented Mar 22, 2023

Thank you @sleepwithcoffee for finalizing that, Outstanding work 👏 🎉

@medikoo medikoo closed this as completed Mar 22, 2023
@sleepwithcoffee sleepwithcoffee mentioned this issue Mar 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cat/dependency good first issue help wanted refactor
Projects
None yet
Milestone
No milestone