fix: clean errors, add validation & reduce exposed #88
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
src/main.js
Outdated
| } | ||
| }); | ||
|
|
||
| app.get('/_ah/health', (request, response) => response.send('OK')); | ||
|
|
||
| app.get('/_ah/stop', async(request, response) => { | ||
| // only expose chrome disable in development |
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.
Let's just remove /_ah/stop entirely.
test/app-test.js
Outdated
| const testBase = 'http://localhost:1234/'; | ||
| process.env.NODE_ENV = 'development'; |
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.
Remove per other comment
src/main.js
Outdated
| const CONFIG_PATH = path.resolve(__dirname, '../config.json'); | ||
| const PROGRESS_BAR_PATH = path.resolve(__dirname, '../node_modules/progress-bar-element/progress-bar.html'); | ||
| const PORT = process.env.PORT || '3000'; | ||
| const ENVIRONMENT = process.env.NODE_ENV; |
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.
Remove per below comment.
test/app-test.js
Outdated
| @@ -39,8 +47,8 @@ test.before(async(t) => { | |||
| async function createServer(config) { | |||
| delete require.cache[require.resolve('../src/main.js')]; | |||
| const app = await require('../src/main.js'); | |||
| if (config) | |||
| app.setConfig(config); | |||
| if (config) app.setConfig(config); | |||
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 we leave this on a separate line please?
test/app-test.js
Outdated
| if (config) | ||
| app.setConfig(config); | ||
| if (config) app.setConfig(config); | ||
| await appInstances.push(app); |
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.
await isn't necessary
|
Covered the changes requested in the latest commit |
This PR fixes a few issues listed below:
/_ah/stopifNODE_ENVis set todevelopmentprogress-bar.htmlused in application