Consolidate .yml files into webpacker.yml #403

gauravtiwari · 2017-05-20T17:46:07Z

Consolidates and flatten all webpacker settings into one yml file - config/webpacker.yml so we have a single source for configuration of instead multiple files, which made things a bit confusing earlier.

  source_path: app/javascript
  source_entry_path: packs
  public_output_path: packs
  
  development: 
    # Only under development
    dev_server:
      host: 0.0.0.0
      port: 8080
      https: false

This is a breaking change and requires a re-install also don't forget to cleanup old files.

bundle update webpacker
bundle exec rails webpacker:install

# Remove old/unused configuration files
rm config/webpack/paths.yml
rm config/webpack/development.server.yml
rm config/webpack/development.server.js

Update ARGV support for webpack-dev-server binstub so you can override dev server host and port from command line. By default 0.0.0.0 is used as host. Also, a new https option is added as well for platforms like cloud9 - https://community.c9.io/t/running-a-rails-app/1615

./bin/webpack-dev-server --host 0.0.0.0

Update webpacker:compile task to use stdout and stderr for logging - Related to #395

Add new changes to README
Update Changelog

Setup eager_load Minor cleanup Remove behind the scenes Update template installer Use filename Install webpack loader as dependency Fix spacing Add watchtoptions Remove hot Remove ARGV for dev server Put back colors Oops use nil Use cheap-module-source-map Minor cleanup Refactor Change to default_settings

javan · 2017-05-21T13:36:30Z

lib/install/bin/webpack-dev-server.tt

+  dev_server  = yaml_config["devServer"]
+
+  WEBPACK_CONFIG  = File.join(APP_PATH, paths["config"], "development.js")
+  DEV_SERVER_HOST = "http#{"s" if dev_server["https"]}://#{dev_server["host"]}:#{dev_server["port"]}"


Instead of dropping ARGV entirely, maybe we should parse it and use the relevant options when supplied. Something along the lines of #{options["host"] || dev_server["host"]}. WDYT?

javan · 2017-05-21T13:38:56Z

lib/install/config/webpacker.yml

+
+default: &default
+  devServer:                  # https://webpack.js.org/configuration/dev-server/
+    enabled: true             # Enabled by default in development


Are we using this option anywhere? What's it supposed to do?

Oh yes, we are not using this anymore after #397. Will remove 👍

javan · 2017-05-21T13:49:25Z

lib/webpacker/configuration.rb

    end

-    def fetch(key)
-      paths.fetch(key, default_paths[key])
-    end


This provided a fallback to the default on a per-key basis. With your change it will only use the default if the entire paths object is absent in an app's webpacker.yml.

Ahh yes, but this will still read from the default config file so this would't be a problem right? Probably missing something

Before, if paths.output was missing in my webpacker.yml file, it would use the default. Now it will only use the default if paths is entirely missing.

javan · 2017-05-21T13:50:36Z

lib/install/config/webpacker.yml

+      - .svg
+      - .gif
+      - .jpeg
+      - .jpg


WDYT about moving extensions out of paths?

Makes sense I think 👍

javan · 2017-05-21T13:53:54Z

lib/install/config/webpacker.yml

+# Note: You must restart bin/webpack-dev-server for changes to take effect
+
+default: &default
+  devServer:                  # https://webpack.js.org/configuration/dev-server/


camelCase is uncommon in other .yml files. Maybe rename to dev_server?

Will fix 👍

javan · 2017-05-21T13:55:10Z

lib/install/config/webpacker.yml

+  devServer:
+    host: example.com
+    port:
+    https: true


Any reason for adding this? Seems like it's encouraging use in production.

Some folks are using custom domain + https. Perhaps, I will change this to a dev domain.

Related to #319

#319 is about using https in development mode if I'm reading it correctly. This is the production config.

Ahh yes my bad, mixed it up with this one - #176. Changed it to 0.0.0.0

I still don't think we should provide any different dev_server options in production. Doing so suggests that we encourage running it in production, which probably isn't a good idea. People can customize if needed.

javan · 2017-05-21T13:59:13Z

lib/install/config/webpacker.yml

+  <<: *default
+
+  paths:
+    manifest: manifest-test.json


Maybe it'd be better to use output: packs-test instead of a different manifest? Then you won't end up with a mix of compiled .js files.

Will fix 👍

javan · 2017-05-21T15:11:00Z

lib/install/config/webpacker.yml

+
+  paths:
+    output: packs-test
+    manifest: manifest-test.json


Don't think we need a different manifest filename if the files are all in separate dir.

javan · 2017-05-21T15:24:18Z

lib/install/config/webpacker.yml

+    entry: packs               # ~/:source/:entry
+    output: packs              # ~/public/:output
+    manifest: manifest.json    # ~/public/:output/:manifest
+    config: config/webpack     # ~/:config


I'd like to propose a flattened set of options that drops paths entirely and helps clarifies each option's meaning (the fact that I added the ~ legend is a smell):

source_path: app/javascript source_entry_path: packs public_output_path: packs

In doing so, we'd drop the manifest and config options. I'm not sure why you'd want/need to change those, but I'm open to keeping them if there's a compelling reason to.

I think it makes sense and since the output directory can be modified the manifest can remain same 👍 Yep, was thinking same for config option - can't think of a scenario this will need changing in any app.

In regards to removing paths, I feel it's much nicer to keep it under some namespace like we have for dev-server - keeps it consistent and clean. WDYT?

Actually, we should probably flatten all of the namespaces. They don't merge into the environments like you'd expect:

>> config = YAML.load_file("webpacker.yml") >> config["default"]["paths"] => {"source"=>"app/javascript", "entry"=>"packs", "output"=>"packs", "manifest"=>"manifest.json", "config"=>"config/webpack"} >> config["test"]["paths"] => {"output"=>"packs-test"}

So the dev server options should be flattened to dev_server_host: ..., dev_server_port: ..., etc. if we want to pick and choose options to override within each env.

Yepp, that would work 👍 Although a bit repetitive and verbose if we start to have many options, especially for the dev server -dev_server-host, dev_server_port, dev_server_hot, dev_server_https. Namespace gives a bit flexibility on the JS side too - it's easy to extract objects using - { dev_server, paths }

default: &default dev_server_host: localhost dev_server_port: 8080 dev_server_https: false dev_server_hot: false source_path: app/javascript source_entry_path: packs public_output_path: packs

VS

default: &default dev_server: # https://webpack.js.org/configuration/dev-server/ host: localhost # Dev server host port: 8080 https: false hot: false paths: # ~ = Rails.root source: app/javascript # ~/:source entry: packs # ~/:source/:entry output: packs # ~/public/:output

The more important part is that those namespaces won't work if you want to change part of them in an environment. For example:

>> config = YAML.load(" default: &default paths: source: app/javascript entry: packs output: packs production: <<: *default paths: output: foo ") => {"default"=>{"paths"=>{"source"=>"app/javascript", "entry"=>"packs", "output"=>"packs"}}, "production"=>{"paths"=>{"output"=>"foo"}}} >> config["production"]["paths"]["source"] => nil >> config["production"]["paths"]["entry"] => nil >> config["production"]["paths"]["output"] => "foo"

Ahh I see 👍

How about this ?

# Note: You must restart bin/webpack-dev-server for changes to take effect dev_server_defaults: &dev_server_defaults # https://webpack.js.org/configuration/dev-server/ host: localhost # Dev server host port: 8080 https: false paths_defaults: &paths_defaults # ~ = Rails.root source: app/javascript # ~/:source entry: packs # ~/:source/:entry output: packs # ~/public/:output default: &default dev_server: <<: *dev_server_defaults paths: <<: *paths_defaults extensions: - .coffee - .js - .jsx - .ts - .vue - .sass - .scss - .css - .png - .svg - .gif - .jpeg - .jpg development: <<: *default test: <<: *default paths: <<: *paths_defaults output: packs-test production: <<: *default dev_server: <<: *dev_server_defaults host: 0.0.0.0 # Cloud9 port: 8080 https: true

Looking again - feels like it will be much better if we just flatten it 👍

Fix typo

javan · 2017-05-21T15:31:12Z

lib/install/config/webpack/development.js

+  devServer: {
+    clientLogLevel: 'none',
+    host: dev_server.host,
+    port: dev_server.port,


Should add the https config here too

Ahh it was there, will fix 👍

gauravtiwari · 2017-05-21T17:28:16Z

@javan Pushed updates. Please take a look when you get a chance.

// Ref - #372

javan · 2017-05-21T18:56:47Z

lib/install/config/webpacker.yml

+
+  dev_server_host: 0.0.0.0
+  dev_server_port: 8080
+  dev_server_https: true


My vote is to remove these alternate dev server settings from production. People can add them if they're using the dev server in production (for some reason). Also, if 0.0.0.0 works more broadly, let's use it in the default settings instead of localhost.

Sounds good 👍

javan · 2017-05-21T19:03:14Z

lib/install/config/webpack/development.js


 module.exports = merge(sharedConfig, {
-  devtool: 'sourcemap',
+  devtool: 'cheap-module-source-map',


Is this a necessary change or just better source map option?

Recommended source map option in development

javan · 2017-05-21T19:12:44Z

lib/install/config/webpacker.yml

+default: &default
+  dev_server_host: 0.0.0.0
+  dev_server_port: 8080
+  dev_server_https: false


Thinking more.. Maybe we should move these options to the development config only since bin/webpack-dev-server exclusive loads config/webpack/development.js (which is the only config file containing devServer).

default: &default source_path: app/javascript source_entry_path: packs public_output_path: packs ... development: <<: *default dev_server: host: 0.0.0.0 port: 8080 https: false ...

Unless someone wanna override it in other environment - cloud9? I guess then we would enter into the same problem we discussed earlier right?

I've never used cloud9, but I gather that the issue in #176 in wasn't about using a different environment, just configuring the host and port. It looks like apps on cloud9 run in development mode: https://community.c9.io/t/running-a-rails-app/1615

Me neither 😄 Alright, makes sense so lets move the dev server to development only.

gauravtiwari · 2017-05-21T21:08:59Z

Guess this is good to go 😄

javan · 2017-05-22T10:49:55Z

Looks great! 👍

Not a new problem, but the "upgrade" process is pretty rough. Running rails webpacker:install leaves the old (and now unused) .yml / .js files lingering in config/webpack. Fixing that is for a separate PR, but might be worth mentioning in the changelog.

javan · 2017-05-22T10:51:57Z

Mind adding a more detailed description of the changes to the PR?

gauravtiwari · 2017-05-22T10:53:47Z

Yepp I have been thinking about that. Sure 👍

guilleiguaran · 2017-05-22T15:24:50Z

Loving this change <3

gauravtiwari · 2017-05-22T15:52:36Z

@javan Over to you 😄 👍

javan · 2017-05-22T16:03:44Z

lib/tasks/webpacker/compile.rake


 namespace :webpacker do
  desc "Compile javascript packs using webpack for production with digests"
  task compile: ["webpacker:verify_install", :environment] do
-    puts "Compiling webpacker assets 🎉"
+    new_line = "\n\n"
+    puts "[Webpacker] Compiling assets 🎉" + new_line


puts implicitly adds a newline. Do we really want 3 total? I'd prefer to drop the new_line var and just use puts on its own line where we want spacing.

gauravtiwari · 2017-05-22T16:08:32Z

Just removed the whole thing - thought it would look better but think it looks alright anyway 😄

javan · 2017-05-22T16:13:33Z

lgtm, merge away!

gauravtiwari · 2017-05-22T16:25:57Z

And merged 🎉 Thanks @javan 👍 Now will update the README so @dhh can make a new release

jfly · 2017-07-21T17:06:19Z

This is a breaking change and requires a re-install also don't forget to cleanup old files.

bundle update webpacker
bundle exec rails webpacker:install

# Remove old/unused configuration files
rm config/paths.yml
rm config/development.server.yml
rm config/development.server.js

I don't have any files called config/paths.yml, config/development.server.yml, or config/development.server.js. I assume these are actually the files in the config/webpack directory.

gauravtiwari · 2017-07-21T18:34:24Z

@jfly Yepp that's correct - https://github.com/rails/webpacker/blob/master/CHANGELOG.md#breaking-change

gauravtiwari requested a review from javan May 20, 2017 17:46

javan reviewed May 21, 2017

View reviewed changes

Remove enabled

eaa5056

javan reviewed May 21, 2017

View reviewed changes

gauravtiwari added 3 commits May 21, 2017 15:00

Change case and move extension out of paths

f03ed31

Change output directory for test

c877358

Add back argv

af4be41

javan reviewed May 21, 2017

View reviewed changes

gauravtiwari added 2 commits May 21, 2017 16:12

Use 0.0.0.0 for cloud 9

58c428d

Remove manifest overide since we don't use different output

1702466

javan reviewed May 21, 2017

View reviewed changes

Revert configuration.rb to use default path-per-key

099f080

Fix typo

javan reviewed May 21, 2017

View reviewed changes

gauravtiwari added 8 commits May 21, 2017 16:34

Add https to dev server

a60d569

Move option up

32f18cd

Remove config and manifest option

91c6c7b

Remove reference to config_path

abc53ac

Flatten webpacker settings

37706a9

Fix extensions

f76db0e

Update readme

1140244

Update changelog

f992607

javan reviewed May 21, 2017

View reviewed changes

gauravtiwari added 2 commits May 21, 2017 20:05

Use 0.0.0.0 for host

5e363f3

Fix typo

bb19d26

javan reviewed May 21, 2017

View reviewed changes

gauravtiwari added 4 commits May 21, 2017 21:05

Move dev_server to development env

6cdf5d0

Change wording

57e082c

Remove contentBase

3046503

Fix typo

9d3288a

gauravtiwari added 2 commits May 22, 2017 16:16

Update changelog and fix compile task

bfab985

Add a link break

c030cf9

Print stdout alongside stderr

6b60766

javan reviewed May 22, 2017

View reviewed changes

gauravtiwari added 2 commits May 22, 2017 17:05

Use puts

6cc0600

Remove extra puts

57e7ecf

gauravtiwari merged commit 158987a into rails:master May 22, 2017

gauravtiwari deleted the consolidate-yml-files branch May 22, 2017 16:23

This was referenced May 22, 2017

Compiles fail with no output #395

Closed

add erb extensions to paths.yml? #262

Closed

Consolidate .yml files into webpacker.yml #403

Consolidate .yml files into webpacker.yml #403

Conversation

gauravtiwari commented May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravtiwari May 21, 2017 • edited Loading

Choose a reason for hiding this comment

gauravtiwari May 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravtiwari commented May 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravtiwari commented May 21, 2017

javan commented May 22, 2017

javan commented May 22, 2017

gauravtiwari commented May 22, 2017

guilleiguaran commented May 22, 2017

gauravtiwari commented May 22, 2017

Choose a reason for hiding this comment

gauravtiwari commented May 22, 2017

javan commented May 22, 2017

gauravtiwari commented May 22, 2017

jfly commented Jul 21, 2017

gauravtiwari commented Jul 21, 2017

gauravtiwari commented May 20, 2017 •

edited

Loading

gauravtiwari May 21, 2017 •

edited

Loading

gauravtiwari May 21, 2017 •

edited

Loading