-
Notifications
You must be signed in to change notification settings - Fork 166
Update to use express for heroku reviews app #326
Conversation
Add procfile to specify heroku server command Update express and package.json Update the app.js script
After these changes, did this help with the memory issue? |
scripts/express/app.js
Outdated
const app = express(); | ||
const port = process.env.PORT || 8081; | ||
// npm script to build assets by webpack | ||
const buildCommand = 'npm run compile:prod'; |
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 we still want to build out our review apps with a dev environment so we get the additional warnings. What do you think?
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.
For now, I'm fine with dev environment with production webpack. As webpack will eat a lot of memories, more dev features will make reviews app more unstable. I'd like to add more features if we can reduce the memory cost with webpack compile.
This update helps us to reduce a lot of the R14 error occurrences. |
@@ -14,7 +14,8 @@ | |||
}, | |||
"homepage": "https://github.com/cerner/terra-core#readme", | |||
"scripts": { | |||
"start": "webpack-dev-server --progress --port ${PORT:-8080}", |
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.
Do we have to lose the port argument? It does help for local 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.
The $PORT
setting's just used for Heroku deployment. I just roll it back after switching to express server.
If it's helpful, I'm fine with just keeping it.
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.
After the post scrum sync, remove PORT
for now. Firstly, currently, we have same PORT config. So we can't open webpack-dev-server and run the npm run test at the same time. We may have two different ports for those and also handle the hot reloading of webpack-dev-server.
Secondly, we can use command line to run dev server on any PORT. That will give developers more control on the command arguments.
Add Procfile to specify heroku server command
Update express and package.json
Update the app.js script
This update helps us to reduce a lot of the R14 error occurrences.
Previous logs: https://gist.github.com/windse7en/e9685f48af1b015ff1245c68297985bc
After updating to express logs: https://gist.github.com/windse7en/b5b0df051286df8970a83c0f3ebfc4e3
Summary
Add
app.js
to run express server frombuild
assets.Procfile
is used to define script to run on heroku.Solved issues
app.listen(port)
at first. It's due to heroku will throw boot timeout error, if the port isn't bound in 60 seconds. To increase this time threshold, we must to contact heroku support. And that only can increase to 120 seconds.