Skip to content

Add dev server config class and setup proxy #637

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

Merged
merged 28 commits into from
Aug 12, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
cbd84c6
Add rack proxy
gauravtiwari Aug 12, 2017
e63fe45
Setup dev_server class
gauravtiwari Aug 12, 2017
3e9ae23
Add and mount the dev_server proxy
gauravtiwari Aug 12, 2017
ea18549
Add tests
gauravtiwari Aug 12, 2017
2cdb999
Update changelog
gauravtiwari Aug 12, 2017
b83b2ad
Update changelog
gauravtiwari Aug 12, 2017
dd4f34a
Fix test env and update tests
gauravtiwari Aug 12, 2017
40a2d04
Only add proxy in development
gauravtiwari Aug 12, 2017
28ea343
Merge branch 'master' into remove-host-from-manifest
gauravtiwari Aug 12, 2017
53c48ad
Merge branch 'master' into remove-host-from-manifest
Aug 12, 2017
87a9df1
Nix extra CR
Aug 12, 2017
e2dd334
Move skipping compile on manifest lookups when dev server is running …
Aug 12, 2017
e1d027a
Give proxy middleware its own initializer
Aug 12, 2017
6f3dca4
Rely on @javan's fix instead of changing all these
Aug 12, 2017
e1261fd
Remove needless local variable
Aug 12, 2017
9ade7b4
Fix compile predicate check
Aug 12, 2017
13fa899
Nix dealing with HTTPS in development
Aug 12, 2017
e5b7def
Only proxy to dev server if its running
Aug 12, 2017
0cdba74
Better grouping
Aug 12, 2017
39a6970
Nix need for config to return anything but full paths
Aug 12, 2017
a915c2b
Supply host_with_port
Aug 12, 2017
0ee002c
Only needed to delete this for HTTPS
Aug 12, 2017
74baa53
Rare occurrence to run dev server in production
Aug 12, 2017
40e3094
Not needed
Aug 12, 2017
1230635
Not needed
Aug 12, 2017
9308b61
Not needed
Aug 12, 2017
8ea5a1b
Update documentation to our new proxy approach
Aug 12, 2017
0b5ae8e
Fix rubocop offenses
gauravtiwari Aug 12, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,31 @@
- `Webpacker::Compiler.fresh?` and `Webpacker::Compiler.stale?` answer the question of whether compilation is needed.
The old `Webpacker::Compiler.compile?` predicate is deprecated.

- Dev server config class that exposes config options through singleton.

```rb
Webpacker.dev_server.running?
```

- Rack middleware proxies webpacker requests to dev server so we can always serve from same-origin and the lookup works out of the box - no more paths prefixing

### Breaking changes

**Note:** requires running `bundle exec rails webpacker:install` and update webpacker.yml

- Move dev-server config options under defaults so it's transparently available in all environments


### Removed

- Host info from manifest.json, now looks like this:

```json
{
"hello_react.js": "/packs/hello_react.js"
}
```

### Fixed

- Update `webpack-dev-server.tt` to respect RAILS_ENV and NODE_ENV values [#502](https://github.com/rails/webpacker/issues/502)
Expand Down
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ gemspec
gem "rails"
gem "rake", ">= 11.1"
gem "rubocop", ">= 0.49", require: false
gem "rack-proxy", require: false

group :test do
gem "minitest", "~> 5.0"
Expand Down
4 changes: 4 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ PATH
specs:
webpacker (2.0)
activesupport (>= 4.2)
rack-proxy (>= 0.6.1)
railties (>= 4.2)

GEM
Expand Down Expand Up @@ -72,6 +73,8 @@ GEM
ast (~> 2.2)
powerpack (0.1.1)
rack (2.0.1)
rack-proxy (0.6.1)
rack
rack-test (0.6.3)
rack (>= 1.0)
rails (5.0.1)
Expand Down Expand Up @@ -131,6 +134,7 @@ DEPENDENCIES
bundler (~> 1.12)
byebug
minitest (~> 5.0)
rack-proxy
rails
rake (>= 11.1)
rubocop (>= 0.49)
Expand Down
35 changes: 4 additions & 31 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -246,40 +246,13 @@ happens when you refer to any of the pack assets using the Webpacker helper meth
That means you don't have to run any separate process. Compilation errors are logged
to the standard Rails log.

If you want to use live code reloading, you'll need to run `./bin/webpack-dev-server`
If you want to use live code reloading, or you have enough JavaScript that on-demand compilation is too slow, you'll need to run `./bin/webpack-dev-server`
in a separate terminal from `./bin/rails server`. This process will watch for changes
in the `app/javascript/packs/*.js` files and automatically reload the browser to match.

Note: The dev server serves pack files from `http://localhost:8080/` by default, which
is a different origin than your application server. Therefore it's not compatible with
things like Service Workers, that requires you to serve from the same origin.

If you'd rather not have to run the two processes separately by hand, you can use [Foreman](https://ddollar.github.io/foreman):

```bash
gem install foreman
```

```yml
# Procfile
web: bundle exec rails s
webpacker: ./bin/webpack-dev-server
```

```bash
foreman start
```

By default, `webpack-dev-server` listens on `0.0.0.0` that means listening
on all IP addresses available on your system: LAN IP address, localhost, 127.0.0.1 etc.
However, we use `localhost` as default hostname for serving assets in browser
and if you want to change that, for example on cloud9 you can do so
by changing host set in `config/webpacker.yml`.

```bash
dev_server:
host: example.com
```
Once you start this development server, Webpacker will automatically start proxying all
webpack asset requests to this server. When you stop the server, it'll revert to
on-demand compilation again.

You can also pass CLI options supported by [webpack-dev-server](https://webpack.js.org/configuration/dev-server/). Please note that inline options will always take
precedence over the ones already set in the configuration file.
Expand Down
3 changes: 1 addition & 2 deletions lib/install/bin/webpack-dev-server.tt
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ begin

HOSTNAME = args('--host') || dev_server["host"]
PORT = args('--port') || dev_server["port"]
HTTPS = ARGV.include?('--https') || dev_server["https"]
DEV_SERVER_ADDR = "http#{"s" if HTTPS}://#{HOSTNAME}:#{PORT}"
DEV_SERVER_ADDR = "http://#{HOSTNAME}:#{PORT}"

rescue Errno::ENOENT, NoMethodError
$stdout.puts "Webpack dev_server configuration not found in #{CONFIG_FILE}."
Expand Down
3 changes: 2 additions & 1 deletion lib/install/config/loaders/core/assets.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ module.exports = {
use: [{
loader: 'file-loader',
options: {
publicPath: output.publicPath,
// Set publicPath with ASSET_HOST env if available so internal assets can use CDN
Copy link
Contributor

Choose a reason for hiding this comment

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

@gauravtiwari can you explain this comment a bit more.

publicPath: output.publicPathWithHost,
name: env.NODE_ENV === 'production' ? '[name]-[hash].[ext]' : '[name].[ext]'
}
}]
Expand Down
3 changes: 2 additions & 1 deletion lib/install/config/webpack/configuration.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ function formatPublicPath(host = '', path = '') {

const output = {
path: resolve('public', settings.public_output_path),
publicPath: formatPublicPath(env.ASSET_HOST, settings.public_output_path)
publicPath: `/${settings.public_output_path}/`.replace(/([^:]\/)\/+/g, '$1'),
Copy link
Contributor

Choose a reason for hiding this comment

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

@gauravtiwari Any chance that you can provide some examples and maybe comments on what's going on here with the formatting of the publicPath.

Since publicPathWithHost uses a function, how about extracting the plain publicPath into a function as well.

Maybe it's me, but doing a string interpolation that feeds into a regexp replace could be a bit less terse for more readability.

I get that you take:

public_output_path = "packs"

and get

public_path ==> "/packs/"

But I'm not following on why you'd be doing something like take "abc//packs/" with a double // in the string, you'd change the double slash to a single slash, and you won't change the protocol.

Is not having slashes in the in the public_output_path an error that should just be reported?

Maybe there's some information missing from the README.md on what you want for the public_output_path?

Copy link
Contributor

Choose a reason for hiding this comment

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

On line 32 below, can you confirm that you're using the adding of the source_path to make non-JS asset inclusion easier? Or are there other reasons?

Copy link
Member Author

Choose a reason for hiding this comment

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

@justin808 I am going to push some cleanup for this whole webpack part in another PR. Basically, the idea was to not end up with // if public_output_path is set to '' i.e. the assets will be outputted at root. Since we are now using proxy, there is no host for publicPath except for internal assets like fonts or images, that's what publicPathWithHost is used for.

On line 32 below, can you confirm that you're using the adding of the source_path to make non-JS asset inclusion easier? Or are there other reasons?

Yes, that's just adding source path to webpack module resolver.

publicPathWithHost: formatPublicPath(env.ASSET_HOST, settings.public_output_path)
}

let resolvedModules = [
Expand Down
1 change: 0 additions & 1 deletion lib/install/config/webpack/development.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ module.exports = merge(sharedConfig, {

devServer: {
clientLogLevel: 'none',
https: settings.dev_server.https,
host: settings.dev_server.host,
port: settings.dev_server.port,
contentBase: output.path,
Expand Down
9 changes: 5 additions & 4 deletions lib/install/config/webpacker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ default: &default
# ['app/assets', 'engine/foo/app/assets']
resolved_paths: []

# webpack-dev-server configuration
dev_server: &dev_server
host: localhost
port: 3035

extensions:
- .coffee
- .erb
Expand All @@ -29,10 +34,6 @@ default: &default
development:
<<: *default
compile: true
dev_server:
host: localhost
port: 8080
https: false

test:
<<: *default
Expand Down
3 changes: 2 additions & 1 deletion lib/webpacker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def instance
end

delegate :logger, :logger=, :env, to: :instance
delegate :config, :compiler, :manifest, :commands, to: :instance
delegate :config, :compiler, :manifest, :commands, :dev_server, to: :instance
delegate :bootstrap, :clobber, :compile, to: :commands
end

Expand All @@ -23,5 +23,6 @@ def instance
require "webpacker/manifest"
require "webpacker/compiler"
require "webpacker/commands"
require "webpacker/dev_server"

require "webpacker/railtie" if defined?(Rails)
12 changes: 8 additions & 4 deletions lib/webpacker/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ def refresh
@data = load
end

def dev_server
fetch(:dev_server)
end

def compile?
fetch(:compile)
end

def source_path
root_path.join(fetch(:source_path))
end
Expand All @@ -33,10 +41,6 @@ def cache_path
root_path.join(fetch(:cache_path))
end

def compile?
fetch(:compile)
end

private
def fetch(key)
data.fetch(key, defaults[key])
Expand Down
35 changes: 35 additions & 0 deletions lib/webpacker/dev_server.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
class Webpacker::DevServer
delegate :config, to: :@webpacker

def initialize(webpacker)
@webpacker = webpacker
end

def running?
Socket.tcp(host, port, connect_timeout: 1).close
true
rescue Errno::ECONNREFUSED, NoMethodError
false
end

def host
fetch(:host)
end

def port
fetch(:port)
end

def host_with_port
"#{host}:#{port}"
end

private
def fetch(key)
config.dev_server.fetch(key, defaults[key])
end

def defaults
config.send(:defaults)[:dev_server]
end
end
23 changes: 23 additions & 0 deletions lib/webpacker/dev_server_proxy.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
require "rack/proxy"

class Webpacker::DevServerProxy < Rack::Proxy
def rewrite_response(response)
status, headers, body = response
headers.delete "transfer-encoding"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @gauravtiwari do you by any chance remember why you needed to delete the Transfer-Encoding header? Maybe something to do with preventing Gzipping?

Doing so breaks chunked streaming as per #2196 (review)

Currently my PR conditionally deletes the header under the assumption that in some cases it does need deleting, but as suggested it would be even better to get rid of the line if not needed :)

response
end

def perform_request(env)
if env["PATH_INFO"] =~ /#{public_output_uri_path}/ && Webpacker.dev_server.running?
env["HTTP_HOST"] = Webpacker.dev_server.host_with_port
super(env)
else
@app.call(env)
end
end

private
def public_output_uri_path
Webpacker.config.public_output_path.relative_path_from(Webpacker.config.public_path)
end
end
4 changes: 4 additions & 0 deletions lib/webpacker/instance.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ def compiler
@compiler ||= Webpacker::Compiler.new self
end

def dev_server
@dev_server ||= Webpacker::DevServer.new self
end

def manifest
@manifest ||= Webpacker::Manifest.new self
end
Expand Down
10 changes: 7 additions & 3 deletions lib/webpacker/manifest.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
class Webpacker::Manifest
class MissingEntryError < StandardError; end

delegate :config, :compiler, :env, to: :@webpacker
delegate :config, :compiler, :env, :dev_server, to: :@webpacker

def initialize(webpacker)
@webpacker = webpacker
Expand All @@ -23,13 +23,17 @@ def refresh
end

def lookup(name)
compile
compile if compiling?
find name
end

private
def compiling?
config.compile? && !dev_server.running?
end

def compile
Webpacker.logger.tagged("Webpacker") { compiler.compile } if config.compile?
Webpacker.logger.tagged("Webpacker") { compiler.compile }
end

def find(name)
Expand Down
9 changes: 9 additions & 0 deletions lib/webpacker/railtie.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
require "rails/railtie"

require "webpacker/helper"
require "webpacker/dev_server_proxy"

class Webpacker::Engine < ::Rails::Engine
initializer "webpacker.proxy" do |app|
if Rails.env.development?
app.middleware.insert_before 0,
Rails::VERSION::MAJOR >= 5 ?
Webpacker::DevServerProxy : "Webpacker::DevServerProxy"
end
end

initializer "webpacker.helper" do |app|
ActiveSupport.on_load :action_controller do
ActionController::Base.helper Webpacker::Helper
Expand Down
11 changes: 11 additions & 0 deletions test/dev_server_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
require "webpacker_test_helper"

class DevServerTest < Minitest::Test
def test_host
assert_equal "localhost", Webpacker.dev_server.host
end

def test_port
assert_equal Webpacker.dev_server.port, 3035
end
end
1 change: 1 addition & 0 deletions webpacker.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Gem::Specification.new do |s|

s.add_dependency "activesupport", ">= 4.2"
s.add_dependency "railties", ">= 4.2"
s.add_dependency "rack-proxy", ">= 0.6.1"

s.add_development_dependency "bundler", "~> 1.12"

Expand Down