refactor: Remove bluebird from lib/classes
#8943
Conversation
|
Hello @juanjoDiaz 👋 Thanks a lot for proposed changes. Before continuing with review, I have a question to @medikoo as I know you're currently working on refactoring variables resolution as a part of #8364 - is the refactor to |
@pgrzesik thanks for asking. I did not touch any part of old implementation. It's that new parser will simply take over, and older will only eventually serve as fallback, in case a new parser will report errors. |
juanjoDiaz
force-pushed
the
remove_bluebird
branch
3 times, most recently
from
c9b6df0
to
93fac95
Compare
Codecov Report
@@ Coverage Diff @@
## master #8943 +/- ##
==========================================
- Coverage 87.69% 87.68% -0.01%
==========================================
Files 268 268
Lines 10050 10045 -5
==========================================
- Hits 8813 8808 -5
Misses 1237 1237
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hello @juanjoDiaz 👋 Thanks a lot - in general it looks good, but according to the original issue, in the first step we should address marking all promise-returning functions as async - here we have more refactoring, but not all promise-returning functions are marked as async. We also have a related PR that addreses that first part for Variables.js here: #8942
To keep things in order, I believe the above PR should be resolved first and then we can continue with that refactoring. What is your opinion here @juanjoDiaz ?
|
Oh I haven't seen that other PR. I think that it makes sense to use Apart from removing BbBluebird, this PR remove unnecessary promise wrappers and replace If you rather waiting for the other PR and then check this, that's also fine. |
There was a problem hiding this comment.
I agree that it makes sense - the problem with these changes is that they often can introduce bugs that are very hard to catch during review process (we already had a few of those caused by async/await refactors), especially when the scope of the PR is bigger, that's why we suggested to migrate it gradually by first marking functions as async, removing unnecessary promise wrapers and only then getting rid of all BbPromise usages. But to not re-do the work you already did I think we should wrap up the changes as a part of this PR - the only thing that would be good here is if you could review the methods that are returning a promise and mark them as async if they're not marked as such yet. I also added a few comments related to Promise.resolve usage that I think we could get rid of
lib/classes/Variables.js
Outdated
| @@ -607,7 +604,7 @@ class Variables { | |||
| if (requestedSlsVar === 'instanceId') { | |||
| valueToPopulate = this.serverless.instanceId; | |||
| } | |||
| return BbPromise.resolve(valueToPopulate); | |||
| return Promise.resolve(valueToPopulate); | |||
There was a problem hiding this comment.
Can we just mark it as async and return the value?
lib/classes/Variables.js
Outdated
| @@ -619,23 +616,23 @@ class Variables { | |||
| } else { | |||
| valueToPopulate = process.env; | |||
| } | |||
| return BbPromise.resolve(valueToPopulate); | |||
| return Promise.resolve(valueToPopulate); | |||
There was a problem hiding this comment.
Can we just mark it as async and return the value?
lib/classes/Variables.js
Outdated
| } | ||
|
|
||
| getValueFromString(variableString) { | ||
| // eslint-disable-line class-methods-use-this | ||
| const valueToPopulate = variableString.replace(/^['"]|['"]$/g, ''); | ||
| return BbPromise.resolve(valueToPopulate); | ||
| return Promise.resolve(valueToPopulate); |
There was a problem hiding this comment.
Can we just mark it as async and return the value?
lib/classes/Variables.js
Outdated
| } | ||
|
|
||
| getValueFromBool(variableString) { | ||
| const valueToPopulate = variableString === 'true'; | ||
| return BbPromise.resolve(valueToPopulate); | ||
| return Promise.resolve(valueToPopulate); |
There was a problem hiding this comment.
Can we just mark it as async and return the value?
lib/classes/Variables.js
Outdated
| } | ||
|
|
||
| getValueFromInt(variableString) { | ||
| const valueToPopulate = parseInt(variableString, 10); | ||
| return BbPromise.resolve(valueToPopulate); | ||
| return Promise.resolve(valueToPopulate); |
There was a problem hiding this comment.
Can we just mark it as async and return the value?
lib/classes/Variables.js
Outdated
| @@ -646,7 +643,7 @@ class Variables { | |||
| } else { | |||
| valueToPopulate = this.options; | |||
| } | |||
| return BbPromise.resolve(valueToPopulate); | |||
| return Promise.resolve(valueToPopulate); | |||
There was a problem hiding this comment.
Can we just mark it as async and return the value?
lib/classes/Variables.js
Outdated
| return BbPromise.reduce( | ||
| deepProperties, | ||
| (reducedValueParam, subProperty) => { | ||
| return Promise.resolve( |
There was a problem hiding this comment.
Let's mark the function as async instead of using Promise.resolve construct
juanjoDiaz
force-pushed
the
remove_bluebird
branch
from
93fac95
to
0ff3ffb
Compare
|
I've marked all the functions returning promises as I have the feeling that quite a few of the async functions in |
There was a problem hiding this comment.
Thank you @juanjoDiaz - awesome job 🙌 However, I believe that I've made a mistake in my previous suggestions as I kinda missed the fact that these functions are not async at all - could you please check if we can just drop async from them? Other than that, we should be good to go 💯
As for any further refactoring - at this point I think we shouldn't really refactor Variables.js as @medikoo is working on introducing totally new variables parser, which should make this one obsolete at some point in the future.
|
Hi @pgrzesik, The function as nor async but are called in a way that things break if you make them sync. I was planning to raise a ticket about that once this PR is merged. |
There was a problem hiding this comment.
Thank you @juanjoDiaz for verifying if we can drop async - all looks great now 🙇
pgrzesik
changed the title
bluebird from lib/classes
Addresses: #7171