-
Notifications
You must be signed in to change notification settings - Fork 30
Fixes usage with serverless-webpack and serverless-offline hooks #1
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
base: master
Are you sure you want to change the base?
Conversation
dankelleher
left a comment
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.
Thanks @martinjlowm , these are really valuable contributions! I have added a couple of comments.
index.js
Outdated
| const functionObject = functionsObject[fn]; | ||
| if (!functionObject.events || functionObject.events.length == 0) { | ||
| const pf = functionProxy(functionObject); | ||
| if (!functionObject.events || !functionObject.events.some((event) => event === 'http')) { |
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.
I think this may be faulty - I just tried it locally and it created proxies for all functions, not just those with no http events (tested with serverless 1.27.3).
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 are right, it is indeed faulty. event is an object with http as key. I'll update the pull request with a fix.
index.js
Outdated
|
|
||
| this.hooks = { | ||
| "before:offline:start:init": this.startHandler.bind(this), | ||
| 'before:offline:start': this.startHandler.bind(this), |
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 seems to work in that it allows me to just run sls offline instead of sls offline start, but sls offline start no longer creates the proxies. Is there any way to support both?
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.
I see. I just tested with both hooks there and it seems like just one of them will trigger when sls offline is invoked with or without start.
… trigger (once) for both `sls offline` and `sls offline start`
|
A short update. The most recent commit is not relevant to the pull request, however, it does at support for AWS XRay tracing. If you are interested in that feature and prefer to have it in its own PR, let me know. |
|
Hi, thanks for the update. I plan to test this PR and get it merged in the next week or so. To keep it clean I'd prefer the Xray tracing on a separate PR if it's not too annoying to pull it out. |
|
I have now moved the XRay commits to a separate PR. :) |
|
Can we merge this in please!!! Super important PR. |
To start off, to my knowledge, the previous hook
offline:start:initdoes not work directly with abeforehandle.Secondly, a function handler location can be customized by the use of
serverless.custom.serverless-offline.location. https://github.com/serverless-heaven/serverless-webpack/ utilizes this to reference bundles in.webpack/servicefor example.Lastly,
httpevents are now checked explicitly, rather than relying on no events at all.