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

Set dev server listen host and port via environment variables #793

Closed
wants to merge 5 commits into from

Conversation

monai
Copy link
Contributor

@monai monai commented Sep 13, 2017

Resolve #767.

@javan
Copy link
Contributor

javan commented Sep 13, 2017

If we're going to support ENV vars, I think we should convert all of bin/webpack-dev-servers ARGVs to them.

@@ -21,11 +21,11 @@ def hot_module_replacing?
end

def host
fetch(:host)
ENV["WEBPACK_DEV_SERVER_LISTEN_HOST"] || fetch(:host)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should all match their method names, e.g. ENV["WEBPACK_DEV_SERVER_HOST"]

@javan javan requested a review from gauravtiwari September 13, 2017 14:52
@gauravtiwari
Copy link
Member

Yepp and may be best to get rid of ARGV support for dev server binstub and add important ones to webpacker.yml itself? Obviously for run time options (host etc.) we can have ENV variables.

@javan
Copy link
Contributor

javan commented Sep 14, 2017

Yeah, I'd like to drop ARGV support in favor of ENV. Maybe like this?

--- a/lib/webpacker/dev_server.rb
+++ b/lib/webpacker/dev_server.rb
@@ -42,7 +42,7 @@ class Webpacker::DevServer
 
   private
     def fetch(key)
-      config.dev_server.fetch(key, defaults[key])
+      ENV["WEBPACK_DEV_SERVER_#{key.upcase}"] || config.dev_server.fetch(key, defaults[key])
     end

@gauravtiwari
Copy link
Member

Yepp looks great 👍

@monai
Copy link
Contributor Author

monai commented Sep 14, 2017

And in binstub we can take all WEBPACK_DEV_SERVER_ variables and merge them with config file. I'm going to update PR in few days.

@gauravtiwari
Copy link
Member

@monai This involves a few more changes so I am going to make another PR that covers everything 👍

@gauravtiwari
Copy link
Member

Closing in favour of #843

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.

3 participants