Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

Update to use express for heroku reviews app #326

Merged
merged 15 commits into from
May 12, 2017
Merged

Conversation

windse7en
Copy link
Contributor

@windse7en windse7en commented May 8, 2017

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

  1. To fix Heroku review apps: memory issue #322 .
    Add app.js to run express server from build assets.
    Procfile is used to define script to run on heroku.
  2. Reduce the time to wake heroku app back to up state.

Solved issues

  1. There is R10 issue without binding 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.

Add procfile to specify heroku server command

Update express and package.json

Update the app.js script
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-326 May 8, 2017 19:03 Inactive
@windse7en windse7en temporarily deployed to terra-core-deployed-pr-326 May 8, 2017 19:18 Inactive
@windse7en windse7en temporarily deployed to terra-core-deployed-pr-326 May 8, 2017 19:51 Inactive
@Matt-Butler
Copy link
Contributor

After these changes, did this help with the memory issue?

const app = express();
const port = process.env.PORT || 8081;
// npm script to build assets by webpack
const buildCommand = 'npm run compile:prod';
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@windse7en windse7en temporarily deployed to terra-core-deployed-pr-326 May 9, 2017 02:20 Inactive
@windse7en windse7en temporarily deployed to terra-core-deployed-pr-326 May 9, 2017 02:26 Inactive
@windse7en
Copy link
Contributor Author

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

@windse7en windse7en temporarily deployed to terra-core-deployed-pr-326 May 9, 2017 17:21 Inactive
@windse7en windse7en temporarily deployed to terra-core-deployed-pr-326 May 9, 2017 18:05 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-326 May 9, 2017 18:49 Inactive
@windse7en windse7en temporarily deployed to terra-core-deployed-pr-326 May 9, 2017 20:45 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-326 May 10, 2017 19:11 Inactive
@@ -14,7 +14,8 @@
},
"homepage": "https://github.com/cerner/terra-core#readme",
"scripts": {
"start": "webpack-dev-server --progress --port ${PORT:-8080}",
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-326 May 11, 2017 12:13 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-326 May 11, 2017 14:58 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-326 May 11, 2017 19:16 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heroku review apps: memory issue
5 participants