feat: Add support for invoke local #258
Conversation
|
The failing test is caused by an unsupported @pgrzesik could you remove the test with node8? |
|
@pgrzesik can you review it? |
|
Hello @CorentinDoue - sorry for the delay here - I'm a bit swamped with other prorities and I'm not that familiar with this plugin's codebase, but I'll try to review it as soon as possible. Thanks for your patience 🙇 |
There was a problem hiding this comment.
Thanks a lot @CorentinDoue and sorry for delayed review. I'm not an expert when it comes to conventions in this repository, but overall the changes look good 👍 I only have a few minor questions/suggestions but we should be good to go soon, please let me know what do you think 🙇
.travis.yml
Outdated
| @@ -101,9 +101,6 @@ jobs: | |||
| - name: 'Unit Tests - Node.js v10' | |||
| node_js: 10 | |||
|
|
|||
| - name: 'Unit Tests - Node.js v8' | |||
There was a problem hiding this comment.
Let's remove that change from this PR, I'll merge it as a separate PR 👍
There was a problem hiding this comment.
ok, I'll drop the commit
| getDataAndContextStub = sinon | ||
| .stub(googleInvokeLocal, 'getDataAndContext') | ||
| .returns(Promise.resolve()); | ||
| invokeLocalStub = sinon.stub(googleInvokeLocal, 'invokeLocal').returns(Promise.resolve()); |
There was a problem hiding this comment.
I believe you can just use .resolves() instead of returns(Promise.resolve()
| let getDataAndContextStub; | ||
| let invokeLocalStub; | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
As the behavior does not change across tests, you can stub it once in before and only reset history after each test with .resetHistory() on stubs.
| this.options[optionKey] = require(absolutePath); | ||
| return; | ||
| } | ||
| this.options[optionKey] = await this.serverless.utils.readFile(absolutePath); |
There was a problem hiding this comment.
Does it depend on the parsing functionality of readFile here? If not, then I would suggest to not use utils.readFile and use native fs to read it
There was a problem hiding this comment.
I tried to make it as iso as possible with the AWS implementation and it loads the files with this util: https://github.com/serverless/serverless/blob/d5e2baf714958c5718610659887f485f9bd161e4/lib/plugins/aws/invokeLocal/index.js#L62
| @@ -7,7 +7,7 @@ module.exports = { | |||
| setDefaults() { | |||
| this.options.stage = | |||
| _.get(this, 'options.stage') || _.get(this, 'serverless.service.provider.stage') || 'dev'; | |||
| this.options.runtime = _.get(this, 'options.runtime') || 'nodejs8'; | |||
There was a problem hiding this comment.
Is that a safe change or it can potentially break for users that are still somehow using nodejs8?
There was a problem hiding this comment.
I think this should have been changed in #244 which drop support for nodejs8. So a nodejs8 user who upgrades this package to 4.0.0 has to explicitly set runtime to 'nodejs8'.
So yes it's another breaking change but it is attached to the breaking change of 4.0.0
There was a problem hiding this comment.
Thanks @CorentinDoue - during release of new version I'll make sure to add that to documentation for 4.0.0 release 👍
CorentinDoue
force-pushed
the
feat/add-invoke-local
branch
from
cf46955 to
95f284d
Compare
CorentinDoue
force-pushed
the
feat/add-invoke-local
branch
from
95f284d to
4ee7b24
Compare
|
@pgrzesik I corrected or answered your remarks. It's ready for a new review |
This pull request adds the implementation of the command
invoke localfor thegoogleprovider. It's inspired by the AWS implementation to keep the behaviors as close as possible.The command is available for the
nodejsruntimes and events of typeeventTests
PR merged to fix it: chore: use processedOptions serverless-heaven/serverless-webpack#834
Next steps
httpevents