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

Allow overriding dev server settings using env variables #843

Merged
merged 1 commit into from Sep 24, 2017

Conversation

Projects
None yet
4 participants
@gauravtiwari
Collaborator

gauravtiwari commented Sep 21, 2017

This PR removes cli args support for webpack dev server and adds support for env variables within Webpacker to override dev server settings.

For overriding dev server settings, simply set env variable before calling webpacker.

WEBPACKER_DEV_SERVER_HMR=true
WEBPACKER_DEV_SERVER_INLINE=true
WEBPACKER_DEV_SERVER_DISABLEHOSTCHECK=false

All settings for dev server inside config/webpacker.yml can be overriden, just make sure they are in caps.

@javan

This comment has been minimized.

Show comment
Hide comment
@javan

javan Sep 21, 2017

Member

Do we need to support dotenv with this change? I'm hesitant to add another dependency, and would prefer to start by just reading ENV (Ruby) / process.env (Node) directly.

Member

javan commented Sep 21, 2017

Do we need to support dotenv with this change? I'm hesitant to add another dependency, and would prefer to start by just reading ENV (Ruby) / process.env (Node) directly.

@gauravtiwari

This comment has been minimized.

Show comment
Hide comment
@gauravtiwari

gauravtiwari Sep 21, 2017

Collaborator

Yeah, I was too but later felt since we are encapsulating configs inside a node module it would make sense to provide env support out of the box. Obviously, we are adding another dependency but I guess the usefulness of this is greater and most of the apps will find this useful. What do you think?

Collaborator

gauravtiwari commented Sep 21, 2017

Yeah, I was too but later felt since we are encapsulating configs inside a node module it would make sense to provide env support out of the box. Obviously, we are adding another dependency but I guess the usefulness of this is greater and most of the apps will find this useful. What do you think?

@javan

This comment has been minimized.

Show comment
Hide comment
@javan

javan Sep 21, 2017

Member

I think people using dotenv (or any env manager for that matter) should just ensure their env vars are set before calling into webpacker.

Member

javan commented Sep 21, 2017

I think people using dotenv (or any env manager for that matter) should just ensure their env vars are set before calling into webpacker.

@gauravtiwari

This comment has been minimized.

Show comment
Hide comment
@gauravtiwari

gauravtiwari Sep 21, 2017

Collaborator

Yeah, totally but it's not that trivial to setup. Here is our doc about supplying env variable to Webpacker: https://github.com/rails/webpacker/blob/master/docs/env.md (which basically requires overriding Environment plugin).

Collaborator

gauravtiwari commented Sep 21, 2017

Yeah, totally but it's not that trivial to setup. Here is our doc about supplying env variable to Webpacker: https://github.com/rails/webpacker/blob/master/docs/env.md (which basically requires overriding Environment plugin).

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Sep 21, 2017

Member
Warning: using Foreman/Invoker and npm dotenv at the same time can result in confusing behavior, in that Foreman/Invoker variables take precedence over npm dotenv variables.

I think this warning explain why we shouldn't add a default env manager and leave it to the users.

Member

guilleiguaran commented Sep 21, 2017

Warning: using Foreman/Invoker and npm dotenv at the same time can result in confusing behavior, in that Foreman/Invoker variables take precedence over npm dotenv variables.

I think this warning explain why we shouldn't add a default env manager and leave it to the users.

@gauravtiwari gauravtiwari changed the title from Add support for env variables to Allow overriding dev server settings using env variables Sep 21, 2017

@gauravtiwari

This comment has been minimized.

Show comment
Hide comment
@gauravtiwari

gauravtiwari Sep 21, 2017

Collaborator

Makes sense 👍

Collaborator

gauravtiwari commented Sep 21, 2017

Makes sense 👍

@gauravtiwari

This comment has been minimized.

Show comment
Hide comment
@gauravtiwari

gauravtiwari Sep 21, 2017

Collaborator

@guilleiguaran Could you please review and merge this one? I have dropped dotenv

Collaborator

gauravtiwari commented Sep 21, 2017

@guilleiguaran Could you please review and merge this one? I have dropped dotenv

@gauravtiwari gauravtiwari merged commit 914a4ea into master Sep 24, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gauravtiwari gauravtiwari deleted the configurable-dev-server-options branch Sep 24, 2017

@timkrins

This comment has been minimized.

Show comment
Hide comment
@timkrins

timkrins Sep 7, 2018

I can understand using ENV variables, but why specifically remove support for ARGV also?

Unable to use https certificates that I would usually expect to pass to webpack-dev-server like so:

webpack-dev-server --https --cert=/home/user/selfsigned.crt --key=/home/user/selfsigned.key

timkrins commented Sep 7, 2018

I can understand using ENV variables, but why specifically remove support for ARGV also?

Unable to use https certificates that I would usually expect to pass to webpack-dev-server like so:

webpack-dev-server --https --cert=/home/user/selfsigned.crt --key=/home/user/selfsigned.key

@gauravtiwari

This comment has been minimized.

Show comment
Hide comment
@gauravtiwari

gauravtiwari Sep 7, 2018

Collaborator

Hey @timkrins

The previous implementation was limited. Webpacker uses dev server options (webpacker.yml or ENV) for an internal rake middleware and it was a bit complex to allow three sources for overriding dev server configuration - cli, webpacker.yml and ENV

This could be unified but haven't had time to look into this properly. For now, you could supply certs through config/webpack/development.js

For example:

// development.js
environment.config.merge({
   devServer: {
    https: {
      key: fs.readFileSync('/path/to/server.key'),
      cert: fs.readFileSync('/path/to/server.crt'),
      ca: fs.readFileSync('/path/to/ca.pem'),
    }
  }
})
Collaborator

gauravtiwari commented Sep 7, 2018

Hey @timkrins

The previous implementation was limited. Webpacker uses dev server options (webpacker.yml or ENV) for an internal rake middleware and it was a bit complex to allow three sources for overriding dev server configuration - cli, webpacker.yml and ENV

This could be unified but haven't had time to look into this properly. For now, you could supply certs through config/webpack/development.js

For example:

// development.js
environment.config.merge({
   devServer: {
    https: {
      key: fs.readFileSync('/path/to/server.key'),
      cert: fs.readFileSync('/path/to/server.crt'),
      ca: fs.readFileSync('/path/to/ca.pem'),
    }
  }
})
@timkrins

This comment has been minimized.

Show comment
Hide comment
@timkrins

timkrins Sep 7, 2018

Hi @gauravtiwari ,

Thanks for the explanation.

I think as the webpacker webpack binstub does accept ARGV I was not expecting the webpack-dev-server binstub to ignore them.

I am aware I can do what you recommended, however, it is just a bit annoying not to be able to have access to do this via the commandline, when I know that webpack-dev-server supports it.

Is there a potential conflict adding ARGV to the execute_cmd function in dev_server_runner.rb? (just like in webpack_runner.rb)
ie.

    cmd = [
      "#{@node_modules_path}/.bin/webpack-dev-server",
      "--config", @webpack_config
    ] + @argv

timkrins commented Sep 7, 2018

Hi @gauravtiwari ,

Thanks for the explanation.

I think as the webpacker webpack binstub does accept ARGV I was not expecting the webpack-dev-server binstub to ignore them.

I am aware I can do what you recommended, however, it is just a bit annoying not to be able to have access to do this via the commandline, when I know that webpack-dev-server supports it.

Is there a potential conflict adding ARGV to the execute_cmd function in dev_server_runner.rb? (just like in webpack_runner.rb)
ie.

    cmd = [
      "#{@node_modules_path}/.bin/webpack-dev-server",
      "--config", @webpack_config
    ] + @argv
@timkrins

This comment has been minimized.

Show comment
Hide comment
@timkrins

timkrins Sep 7, 2018

Ah, I can see I'm not the only one confused by this - just found #1672

timkrins commented Sep 7, 2018

Ah, I can see I'm not the only one confused by this - just found #1672

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment