Skip to content
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

TypeError: Cannot read property 'action' of undefined #15

Closed
mrchief opened this issue Mar 6, 2019 · 5 comments
Closed

TypeError: Cannot read property 'action' of undefined #15

mrchief opened this issue Mar 6, 2019 · 5 comments

Comments

@mrchief
Copy link
Contributor

mrchief commented Mar 6, 2019

The code at https://github.com/probot/serverless-lambda/blob/master/index.js#L51 doesn't account for cases where event.body could be absent (e.g. using custom routes).

I think a check for event.body.action would at least avoid this error.

A better fix would be to account for other routes and invoke next() instead of trying to handle it within probot.

Stack trace
  at <path/to/repo>\node_modules\@probot\serverless-lambda\index.js:51:50
    at nameSpace.run (<path/to/repo>\node_modules\node-lambda\lib\main.js:125:22)
    at Namespace.run (<path/to/repo>\node_modules\continuation-local-storage\context.js:48:5)
    at Lambda._runHandler (<path/to/repo>\node_modules\node-lambda\lib\main.js:123:15)
    at Lambda.run (<path/to/repo>\node_modules\node-lambda\lib\main.js:84:10)
    at Command.program.command.alias.description.option.option.option.option.option.option.option.option.action (<path/to/repo>\node_modules\node-lambda\bin\node-lambda:138:27)
    at Command.listener (<path/to/repo>\node_modules\commander\index.js:315:8)
    at emitTwo (events.js:126:13)
    at Command.emit (events.js:214:7)
    at Command.parseArgs (<path/to/repo>\node_modules\commander\index.js:654:12)
    at Command.parse (<path/to/repo>\node_modules\commander\index.js:474:21)
    at Object.<anonymous> (<path/to/repo>\node_modules\node-lambda\bin\node-lambda:149:4)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:188:16)
    at bootstrap_node.js:609:3
@JasonEtco
Copy link
Member

Can you tell us more about your use-case? This is intended to be specifically for Probot apps, so I'm not sure why you'd need to account for cases where event.body is absent, it would always be present for GitHub-sent events. If you're looking to use one function to support multiple routes, you should probably be using multiple functions instead.

@mrchief
Copy link
Contributor Author

mrchief commented Mar 6, 2019

They are not new functions, rather routes for things like health checks, statuses etc.

It seems that probot supports it https://probot.github.io/docs/http/.

Also, one can configure probot to receive events at a specific route via WEBHOOK_PATH env var, so this error would happen for those cases as well.

In another case, I'm trying to port an existing app, and need to support existing routes: https://github.com/ewolfe/prlint/blob/feat/probot-app/src/routes.js.

Finally, as I was writing this reply, I'm seeing this note from readme in a new light. :)

It'd be nice if you can support these, if not I'll try to follow the recommendation specified in the readme.

Probot
GitHub Apps to automate and improve your workflow
GitHub
GitHub App for linting pull request meta data. Contribute to ewolfe/prlint development by creating an account on GitHub.

@JasonEtco
Copy link
Member

Yeah, that link to the README makes sense. I think its important to keep this extension very scoped - to follow standard FaaS architecture patterns, and to ensure that we're not shipping unnecessary code (in serverless functions, bundle sizes counts towards execution speed). I'm going to close this issue because I think your question is answered but feel free to reopen if I'm mistaken!

@mrchief
Copy link
Contributor Author

mrchief commented Mar 6, 2019

Can't argue with bundle size and execution speed arguments! :)

I think in the very least, we should at least add prominent warnings in the readme about using custom routes or using WEBHOOK_PATH env var.

@JasonEtco
Copy link
Member

Couldn't hurt - PRs welcome @mrchief ❤️

mrchief added a commit to mrchief/serverless-lambda that referenced this issue Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants