Add dev server config class and setup proxy #637

gauravtiwari · 2017-08-12T11:13:09Z

This PR adds a few things related to webpack-dev-server:

A dev server config class that exposes config options

Webpacker.dev_server.running?

A rack middleware that 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

   if Rails::VERSION::MAJOR >= 5
      app.middleware.insert_before 0, Webpacker::DevServerProxy, ssl_verify_none: true
    else
      config.middleware.insert_before 0, "Webpacker::DevServerProxy", ssl_verify_none: true
    end

HTTPS ssl settings for webpack-dev-server

# Absolute paths to ssl key and certificate
    ssl_key_path:
    ssl_cert_path:

Move dev-server-settings to defaults so it's transparently available in all environments
The host info has been removed from manifest.json, now looks like this:

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

Add options for ssl and set publicPath to only pack output path

Add dev_server config

Move dev-server to defaults

Move to private Fix paths Update path names

Fix typos Delete if path exists

Remove extra if

gauravtiwari · 2017-08-12T15:30:55Z

There is one extra bit of change here i.e. renaming packs to packs-test inside tests - apparently CI is setting RAILS_ENV when running tests and due to output folder being different in webpacker.yml the tests were failing however when running same tests locally the app was booted in development environment.

So I have setup the RAILS.env to be test to avoid any discrepancy:

module TestApp
  class Application < ::Rails::Application
    config.root = File.join(File.dirname(__FILE__), "test_app")
    config.eager_load = true
    ::Rails.env = ENV["RAILS_ENV"] = "test"
  end
end

and renamed all test_app folders from packs to packs-test.

dhh · 2017-08-12T18:46:02Z

lib/install/config/webpack/development.js

+    key: readFileSync(devServer.ssl_key_path),
+    cert: readFileSync(devServer.ssl_cert_path)
+  }
+}


When do we ever need to bother with HTTPS in development?

@dhh There were some issues filed where people are using HTTPS for development, mainly for cases where they are interacting with API's that is hosted on HTTPS

…to manifest

dhh · 2017-08-12T19:15:09Z

Links?

…

On Sat, Aug 12, 2017 at 2:03 PM, Gaurav Tiwari ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/install/config/webpack/development.js <#637 (comment)>: > const sharedConfig = require('./shared.js') const { settings, output } = require('./configuration.js') +const { dev_server: devServer } = settings +let https = devServer.https + +// Set ssl key and certificates if supplied +if (devServer.ssl_key_path && devServer.ssl_cert_path) { + https = { + key: readFileSync(devServer.ssl_key_path), + cert: readFileSync(devServer.ssl_cert_path) + } +} @dhh <https://github.com/dhh> There were some issues filed where people are using HTTPS for development, mainly for cases where they are interacting with API's that is hosted on HTTPS — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#637 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtSV_zs7FhmrhyGctSSlNyyWVOPzyks5sXfb5gaJpZM4O1Yxc> .

gauravtiwari · 2017-08-12T19:18:02Z

#612 and #319

gauravtiwari · 2017-08-12T19:26:30Z

@dhh Should we move the proxy middleware to middlewares folder, guess we gonna have one more middleware for cache-control.

dhh · 2017-08-12T19:29:05Z

What do we need for cache-control?

Don't need to advertise that so loudly.

gauravtiwari · 2017-08-12T19:37:08Z

For expiring cached assets: #267 and #550

dhh · 2017-08-12T19:42:05Z

Ah yes. Let's do a separate PR for that.

gauravtiwari · 2017-08-12T19:42:25Z

Yepp 👍

dhh · 2017-08-12T19:45:17Z

Actually, if we just use -hash in dev as well, we shouldn't need to explicitly set any headers? But not sure how that'll combine with the dev server.

gauravtiwari · 2017-08-12T19:58:59Z

Yeah that will work 👍 I doubt this is a problem when using dev-server because it takes care of settings cache-control. I guess this is happening mainly with watcher, where assets are being served by Rails server and getting cached but -hash can totally fix it

dhh · 2017-08-12T20:12:48Z

For SSL, we should double down on the reproxy setup. The dev server should not need to know about this. Just like people use nginx to terminate SSL before talking to a puma server.

justin808

@gauravtiwari I have a few questions for you.

justin808 · 2017-08-13T23:26:48Z

lib/install/config/loaders/core/assets.js

@@ -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


@gauravtiwari can you explain this comment a bit more.

justin808 · 2017-08-13T23:52:03Z

lib/install/config/webpack/configuration.js

@@ -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'),


@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?

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?

@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.

sfcgeorge · 2019-07-30T22:05:52Z

lib/webpacker/dev_server_proxy.rb

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


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 :)

gauravtiwari added 5 commits August 12, 2017 11:29

Add rack proxy

cbd84c6

Add options for ssl and set publicPath to only pack output path

Setup dev_server class

e63fe45

Add dev_server config

Add and mount the dev_server proxy

3e9ae23

Move dev-server to defaults

Add tests

ea18549

Move to private Fix paths Update path names

Update changelog

2cdb999

gauravtiwari requested a review from dhh August 12, 2017 11:13

Update changelog

b83b2ad

Fix typos Delete if path exists

gauravtiwari force-pushed the remove-host-from-manifest branch from 1acace9 to b83b2ad Compare August 12, 2017 11:26

gauravtiwari added 2 commits August 12, 2017 16:15

Fix test env and update tests

dd4f34a

Only add proxy in development

40a2d04

Remove extra if

gauravtiwari force-pushed the remove-host-from-manifest branch from ce1ad37 to 40a2d04 Compare August 12, 2017 15:21

gauravtiwari and others added 2 commits August 12, 2017 16:52

Merge branch 'master' into remove-host-from-manifest

28ea343

Merge branch 'master' into remove-host-from-manifest

53c48ad

dhh reviewed Aug 12, 2017

View reviewed changes

dhh added 7 commits August 12, 2017 13:52

Nix extra CR

87a9df1

Move skipping compile on manifest lookups when dev server is running …

e2dd334

…to manifest

Give proxy middleware its own initializer

e1d027a

Rely on @javan's fix instead of changing all these

6f3dca4

Remove needless local variable

e1261fd

Fix compile predicate check

9ade7b4

Nix dealing with HTTPS in development

13fa899

dhh added 4 commits August 12, 2017 14:18

Only proxy to dev server if its running

e5b7def

Better grouping

0cdba74

Nix need for config to return anything but full paths

39a6970

Supply host_with_port

a915c2b

dhh added 5 commits August 12, 2017 14:29

Only needed to delete this for HTTPS

0ee002c

Rare occurrence to run dev server in production

74baa53

Don't need to advertise that so loudly.

Not needed

40e3094

Not needed

1230635

Not needed

9308b61

dhh and others added 2 commits August 12, 2017 14:37

Update documentation to our new proxy approach

8ea5a1b

Fix rubocop offenses

0b5ae8e

dhh merged commit 68b9f08 into master Aug 12, 2017

dhh deleted the remove-host-from-manifest branch August 12, 2017 19:42

justin808 reviewed Aug 14, 2017

View reviewed changes

jakeNiemiec mentioned this pull request Jul 30, 2019

Fix proxy for chunked streaming templates #2196

Merged

sfcgeorge reviewed Jul 30, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dev server config class and setup proxy #637

Add dev server config class and setup proxy #637

gauravtiwari commented Aug 12, 2017 •

edited

Loading

gauravtiwari commented Aug 12, 2017 •

edited

Loading

dhh Aug 12, 2017

gauravtiwari Aug 12, 2017

dhh commented Aug 12, 2017 via email

gauravtiwari commented Aug 12, 2017

gauravtiwari commented Aug 12, 2017

dhh commented Aug 12, 2017

gauravtiwari commented Aug 12, 2017

dhh commented Aug 12, 2017

gauravtiwari commented Aug 12, 2017

dhh commented Aug 12, 2017

gauravtiwari commented Aug 12, 2017

dhh commented Aug 12, 2017

justin808 left a comment

justin808 Aug 13, 2017

justin808 Aug 13, 2017

justin808 Aug 14, 2017

gauravtiwari Aug 14, 2017

sfcgeorge Jul 30, 2019

Add dev server config class and setup proxy #637

Add dev server config class and setup proxy #637

Conversation

gauravtiwari commented Aug 12, 2017 • edited Loading

gauravtiwari commented Aug 12, 2017 • edited Loading

dhh Aug 12, 2017

Choose a reason for hiding this comment

gauravtiwari Aug 12, 2017

Choose a reason for hiding this comment

dhh commented Aug 12, 2017 via email

gauravtiwari commented Aug 12, 2017

gauravtiwari commented Aug 12, 2017

dhh commented Aug 12, 2017

gauravtiwari commented Aug 12, 2017

dhh commented Aug 12, 2017

gauravtiwari commented Aug 12, 2017

dhh commented Aug 12, 2017

gauravtiwari commented Aug 12, 2017

dhh commented Aug 12, 2017

justin808 left a comment

Choose a reason for hiding this comment

justin808 Aug 13, 2017

Choose a reason for hiding this comment

justin808 Aug 13, 2017

Choose a reason for hiding this comment

justin808 Aug 14, 2017

Choose a reason for hiding this comment

gauravtiwari Aug 14, 2017

Choose a reason for hiding this comment

sfcgeorge Jul 30, 2019

Choose a reason for hiding this comment

gauravtiwari commented Aug 12, 2017 •

edited

Loading

gauravtiwari commented Aug 12, 2017 •

edited

Loading