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

Make haste-server work out of the box with default configuration and overrides + Heroku/Dokku Support #7

Closed
wants to merge 3 commits into from

Conversation

tmfksoft
Copy link

This change moves from using the config.js import to using config.
This allows haste-server to work out of the box with a simple npm i and npm start.

The by-product of this and primary reason I made these changes + PR is to allow haste-server to be thrown into Heroku/Dokku with zero effort.

No configuration changes were required other than adding a default production.js that respects process.env.PORT and process.env.HOST for Heroku/Dokku support.

You can find a copy of this PR running in Heroku and Dokku here:
https://tmfksoft-haste.herokuapp.com/
https://haste.dokku.burnett-taylor.me/

@tmfksoft
Copy link
Author

I forgot to mention, due to there being no changes to the configuration at all.
Existing users can simply mv their existing config.js to config/development.js or config/production.js

@zneix
Copy link
Owner

zneix commented Sep 20, 2020

Hello and thanks for contribution!

Sorry for late response, I had to make up my mind first. While I see that you (and probably many other people) want out-of-box support, I am not a fan of adding extra dependencies, especially when it's easily possible to avoid adding them.

I never thought of running haste just like this without any initial configuration - we can easily support that but I dislike solution you provided.
I came up with an idea of solving that on testing branch in 58c37bf, code there:

  • Copies example.config.js to config.js if second file doesn't exist. That should cover out-of-the-box support.
  • HASTE_HOST and HASTE_PORT env variables are respected over settings in config.

Please let me know what do you think of my solution and I can merge it to master later if it's covering your needs.

@tmfksoft
Copy link
Author

Thanks for getting back to me,

I thought you'd not be 100% on the extra dependency, I added it as an easy and clean way to manage configs.

Your changes would be fine, the only thing that needs to be respected primarily is "process.env.PORT" as this is what Heroku, Dokku etc expose to instruct the application to listen on the correct port for reverse proxying into the container the app runs within. If HASTE_PORT and HASTE_HOST were changed to PORT and HOST respectively then it should work fine.

Other than that, that'd work perfectly.

zneix added a commit that referenced this pull request Sep 20, 2020
Removed HASTE_ prefix as it would not work on evironments like Heroku, relevant comment: #7 (comment)
@zneix
Copy link
Owner

zneix commented Sep 20, 2020

the only thing that needs to be respected primarily is "process.env.PORT" as this is what Heroku, Dokku etc expose to instruct the application to listen on the correct port for reverse proxying into the container the app runs within

Alright, I wanted to use some kind of prefix to avoid possible collisions with already existing apps / environments on a bigger server but since Heroku / Dokku need them to be specifically PORT and HOST to run properly, I changed those on test branch.

Sorry that your Pull Request turned out like this, but unless you have any questions/comments about it, I can merge my changes to master and close your PR. Cheers!

@zneix zneix closed this Sep 20, 2020
@tmfksoft
Copy link
Author

Yup, that's fine.
Not fussed about the PR, just the outcome!

Thanks for continuing Haste and taking a look at the PR! :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants