Make GET /probot template more flexible

[swain]

As is, the behavior of the GET /probot HTML template is brittle and non-flexible:

// views/probot.js
const { name, version } = require(`${process.cwd()}/package`);

module.exports.template = `
  ... 
`

// index.js
const { template } = require("./views/probot");
...

// `template` is rendered on `GET /probot`

1. It relies on the presence of a package module in process.cwd() at import time. This reliance seems unnecessary, and is also undocumented in the README.

2. This require statement is not compatible with webpack, which cannot handle a dynamic import like this.

Thoughts on updating this behavior to be configurable, or swapping the require for a safer file read?

[gr2m]

Thank you for opening the issue!

We are currently working on making Probot work better with serverless environments out of the box, see probot/probot#1286 (comment)

The API will look something like this

const { createLamdaMiddleware, createProbot } = require('@probot/lambda')
const myApp = require('../app')

module.export = createLamdaMiddleware(myApp, { probot: createProbot() })

Question: would you expect the returned middleware to handle the GET /probot route at all? The question is if the middleware should only care about the webhook requests from GitHub, while you can define all other routes depending on what you need for your app. Would that resolve your problem?

[swain]

Hi @gr2m! Thanks for the prompt response.

would you expect the returned middleware to handle the GET /probot route at all?

This route is not really important to me or my organization, so removing it wouldn't bother me in the slightest!

However, I think the biggest problem for us was the package's internal reliance on process.cwd() and the presence of a package.json in the deployed lambda, as this didn't play well with our deployment strategies (e.g. use of webpack, not including package.json).

[gr2m]

okay thanks for sharing these insights.

So I think the solution will be to just remove the view by default, which is the case with the new serverless APIs.

Okay to close this issue?

[swain]

@gr2m My notifications are messed up -- thanks for closing, and for addressing!

[swain]

@gr2m FWIW -- it looks like this issue is actually still present in the latest version of this package.

I'm going to put up a PR with a more detailed description.

[swain]

@gr2m PR here, if you're interested: #120.