-
Notifications
You must be signed in to change notification settings - Fork 416
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.
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
add hook *before:step-functions-offline:start* #313
Conversation
Hey @vkkis93 , nice addition 👍 Will be part of the next release. Are there any caveats if using step-functions offline together with serverless-offline, or is that not possible? |
index.js
Outdated
@@ -153,6 +153,9 @@ class ServerlessWebpack { | |||
.then(this.prepareOfflineInvoke) | |||
.then(this.wpwatch), | |||
|
|||
'before:step-functions-offline:start': () => BbPromise.bind(this) | |||
.then(this.validate) | |||
.then(this.compile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watch will automatically watch for source changes. Both functionalities (offline watch and invoke local watch) set the base directory for the execution properly, e.g. for serverless-offline the location parameter is set correctly.
If there is something similar for the step-functions that needs to be done, we had to add a different "watch" or "prepare" function for step (e.g. stepWatch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using .then(this.validate).then(this.compile) need to create separate function for plugin ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if we want to support watch mode and if step-offline needs to know the compiled directory, then you can do:
.then(this.validate)
.then(this.prepareStepOffline) <<<--- see here
.then(this.wpwatch)
The new prepareStepOffline
function should then do something similar as https://github.com/serverless-heaven/serverless-webpack/blob/master/lib/prepareOfflineInvoke.js, only that it prepares the root folder for the step-offline plugin.
Otherwise the step-offline will just run on the uncompiled code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need only compile code once, run step-offline and stop process.
I don't need to have watch mode
About compiled directory, user will indicate the path to directory by himself in plugin settings.
Referring to my requirements - need to change something or this code makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense with the compile, yes.
However:
About compiled directory, user will indicate the path to directory by himself.
That will not really work out, if you set package: individually. Then the compiled code will then be distributed. I think you need a "prepare" function because for the offline, it has to be set to service packaging (then the output directory is ".webpack/service") like in the prepareOffline.
With individual packaging it is ".webpack/" which is not predictable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give me the link to your plugin. Maybe I have some time over the weekend to have a look, if we can combine this in an easy and compatible way there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HyperBrain but looks like your approach very good.
Makes sense to rewrite it.
Only one question, serverless-webpack compiles all functions which described in section functions in serverless.yml ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great,
I update this pull request to Is this ready for review?: NO at this moment.
When I will rewrite code, I will notify you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I'm already excited 😄
Hi @HyperBrain . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vkkis93 , happy to see this done now ;-)
Please add a unit test (similar to the one for serverless-offline prepare) to have the coverage up again.
index.js
Outdated
@@ -153,6 +155,10 @@ class ServerlessWebpack { | |||
.then(this.prepareOfflineInvoke) | |||
.then(this.wpwatch), | |||
|
|||
'before:step-functions-offline:start': () => BbPromise.bind(this) | |||
.then(this.validate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already spawn the webpack:validate
in the prepareStepOfflineInvoke function, so you can remove the line here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, you're right.
I missed that
@HyperBrain can you show the example of unit tests for serverless-offline ? |
Oops. Just saw that these unit test are also missing. I will add the prepare unit tests for StepOffline to this PR, so you don't have to take care of them 😄 |
@HyperBrain great news |
Hi @HyperBrain . |
I'll finish it tomorrow and merge it. Sorry for the delay, was busy over the weekend ;-) |
lib/prepareStepOfflineInvoke.js
Outdated
const path = require('path'); | ||
|
||
/** | ||
* Special settings for use with serverless-offline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be step-functions-offline
instead of serverless-offline
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
Should be serverless-step-functions-offline (it's name of plugin)
Thanks a lot |
The new implementation is now covered 100%. The decrease is because a different unrelated file is not correctly tested. I will merge this PR now and fix the other unit test in a separate PR. |
@vkkis93 Good job! |
👍 Looking forward to |
Released with |
add hook *before:step-functions-offline:start*
What did you implement:
Integration serverless-step-functions-offline with serverless-webpack
How did you implement it:
Just add hook 'before:step-functions-offline:start'
How can we verify it:
Run serverless-step-functions-offline plugin in serverless project where serverless-webpack will be first runned
Todos:
Is this ready for review?: YES
Is it a breaking change?: NO