refactor: Remove bluebird from lib/plugins/plugin
#8984
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8984 +/- ##
==========================================
+ Coverage 86.31% 87.02% +0.70%
==========================================
Files 274 285 +11
Lines 10241 10902 +661
==========================================
+ Hits 8840 9487 +647
- Misses 1401 1415 +14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you @juanjoDiaz 🙇 It looks good overall, I only have a few minor comments, please let me know what do you think
lib/plugins/plugin/install.js
Outdated
| await this.addPluginToServerlessFile(); | ||
| await this.installPeerDependencies(); | ||
|
|
||
| const message = [ |
There was a problem hiding this comment.
I know it's not strictly part of BbPromise refactoring, but while we're at it - could you also simplify construction of that message? I don't think we need join here and putting it all in one line should be okay.
lib/plugins/plugin/install.js
Outdated
|
|
||
| // install the package through npm | ||
| const pluginFullName = `${this.options.pluginName}@${this.options.pluginVersion}`; | ||
| const message = [ |
There was a problem hiding this comment.
I know it's not strictly part of BbPromise refactoring, but while we're at it - could you also simplify construction of that message? I don't think we need join here and putting it all in one line should be okay.
lib/plugins/plugin/install.js
Outdated
| this.serverless.cli.log(message); | ||
| }); | ||
| }); | ||
| await this.validate(); |
There was a problem hiding this comment.
does it have to be awaited? I believe it does not return a Promise after your changes in utils.js
There was a problem hiding this comment.
I think this wasn't addressed
| @@ -62,7 +66,7 @@ describe('PluginInstall', () => { | |||
| let installStub; | |||
|
|
|||
| beforeEach(() => { | |||
| installStub = sinon.stub(pluginInstall, 'install').returns(BbPromise.resolve()); | |||
| installStub = sinon.stub(pluginInstall, 'install').returns(Promise.resolve()); | |||
There was a problem hiding this comment.
can we replace it with .resolves instead of returns(Promise.resolve())? It is repeated in a few other places as well
juanjoDiaz
force-pushed
the
remove_bluebird_plugins_plugin
branch
from
629fe97
to
f9c4295
Compare
|
Done! |
There was a problem hiding this comment.
thank you @juanjoDiaz 🙇 I believe there's one comment that wasn't addressed - it's a minor thing and after that we should be good to go 💯
lib/plugins/plugin/install.js
Outdated
| this.serverless.cli.log(message); | ||
| }); | ||
| }); | ||
| await this.validate(); |
There was a problem hiding this comment.
I think this wasn't addressed
juanjoDiaz
force-pushed
the
remove_bluebird_plugins_plugin
branch
from
f9c4295
to
c2d0cb0
Compare
pgrzesik
changed the title
lib/plugins/pluginbluebird from lib/plugins/plugin
Addresses: #7171
I'll keep them coming