Consolidate configuration into one file and setup caching for manifest #292

gauravtiwari · 2017-04-26T19:01:12Z

Reload Webpacker manifest when `cache_manifest` is set

# config/webpack/configuration.yml
# Note: You must restart bin/webpack-dev-server for changes to take effect

default: &default
  cache_manifest: false

guilleiguaran · 2017-04-26T21:08:06Z

lib/webpacker/file_loader.rb

    def load(path = file_path)
      self.instance = new(path)
    end
+
+    def reloadable?
+      ["development", "test"].include?(Rails.env)


I think is more common use !Rails.env.production? in Rails code base but we can check

Yepp thought about that too but I only wanted to limit this feature explicitly for - test and development. For ex - folks also might have staging and other environments where they don't want to reload. What do you think?

I think environment is the wrong thing to check here exactly because of that. People may want to enable that in staging, or in an other environment. I'd introduce a new configuration like reload_webpacker with default to false and have this config set to true in the development and test environment files. Just like sprockets does with debug option for example.

Makes sense. Will fix 👍

gauravtiwari · 2017-04-27T06:45:34Z

@guilleiguaran @rafaelfranca Please take a look 👍

rafaelfranca · 2017-04-27T07:06:40Z

lib/webpacker/railtie.rb

@@ -12,6 +13,10 @@ class Webpacker::Engine < ::Rails::Engine
      include Webpacker::Helper
    end

+    ActiveSupport.on_load :before_eager_load do
+      app.config.x.webpacker[:reload] = false


instead of using the x namespace what about creating a webpacker namespace? Also you don't need to use ActiveSupport.on_load :before_eager_load do. The railtie can set the value in its body.

Yepp lets do that, now is the good time apparently 😄 Gotcha 👍

rafaelfranca · 2017-04-27T07:09:30Z

lib/webpacker/file_loader.rb

    def load(path = file_path)
      self.instance = new(path)
    end
+
+    def reloadable?
+      Rails.configuration.x.webpacker[:reload]


Instead of coupling webpacker with the Rails, we should be pushing the configuration to Webpacker::FileLoader from inside the Railtie.

initializer :webpacker do |app| Webpacker::FileLoade.reloadable = app.config.webpacker.reload

Makes sense 👍 Will fix :)

dhh · 2017-04-27T14:00:14Z

lib/webpacker/file_loader.rb

@@ -7,9 +7,17 @@ class FileLoaderError < StandardError; end
  attr_accessor :data

  class << self
+    def exist?
+      instance.data.present? rescue false


The instance or the data not being there isn't really exceptional or unexpected, so let's use try or & to invocate rather than exception handling.

rafaelfranca · 2017-04-27T18:45:56Z

lib/webpacker/file_loader.rb

  attr_accessor :data

  class << self
+    reloadable = false


Should not this be self.reloadable = false? Otherwise it will only define a local variable.

rafaelfranca · 2017-04-27T18:46:37Z

lib/webpacker.rb

@@ -1,4 +1,7 @@
 module Webpacker
+  mattr_accessor :reload_webpacker


Do we need this one? I doesn't seems used.

Still work in progress, I guess pushed it by mistake 😄

dhh · 2017-05-01T10:29:56Z

I'd go with config.webpacker.caching to match config.action_controller.perform_caching. cache alone sounds like where the actual caching instance would live. But otherwise good.

gauravtiwari · 2017-05-01T11:10:59Z

Yepp makes sense. Will fix 👍

gauravtiwari · 2017-05-01T11:35:31Z

Should be good to go now. Updated the PR description to reflect the changes done here 😄

dhh · 2017-05-01T16:50:51Z

CHANGELOG.md

+Webpacker::Configuration.exist? # => true
+```
+
+- Add `reloadable?` helper method to check if configuration or manifest is reloaded.


What is this used for?

Nowhere at the moment, just added it as a reverse alias to Webpacker.caching in case someone wants to use this instead. Probably more readable?

dhh · 2017-05-01T18:49:56Z

Let's nix code we don't use and program for today. Also need to generally explain the benefit of not caching.

…

On Mon, May 1, 2017 at 9:53 AM, Gaurav Tiwari ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In CHANGELOG.md <#292 (comment)>: > +```rb +# config/environments/production.rb +config.webpacker.caching = true + +# config/environments/staging.rb +config.webpacker.caching = false +``` + +- Add `exist?` helper method to check if configuration or manifest data exist. + +```rb +Webpacker::Manifest.exist? # => false +Webpacker::Configuration.exist? # => true +``` + +- Add `reloadable?` helper method to check if configuration or manifest is reloaded. Nowhere at the moment, just added it as an alias to Webpacker.caching in case someone wants to use this instead. Probably more readable? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#292 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtRPTUZxnC16gb0ecrlOIEpNZNLA9ks5r1g37gaJpZM4NJSgm> .

Remove redundant assign Fix comment Fix sentence

gauravtiwari · 2017-05-10T17:10:40Z

@dhh This one is ready too 👍

javan · 2017-05-10T18:47:09Z

I'd go with config.webpacker.caching to match config.action_controller.perform_caching. cache alone sounds like where the actual caching instance would live. But otherwise good.

This change is more similar to config.cache_classes than config.action_controller.perform_caching. Maybe config.webpacker.cache_configuration instead?

gauravtiwari · 2017-05-10T18:56:25Z

@javan We are caching both configuration and manifest here so, not sure if the name should include particular class (instead be more generic)

javan · 2017-05-10T18:59:10Z

I would expect config.webpacker.caching to cache compiled assets or something. I wonder if we should just use config.cache_classes from Rails to determine this.

dhh · 2017-05-10T19:00:26Z

Don't want to overload cache_classes. That's specifically for the Ruby auto loader etc.

…

On May 10, 2017, at 20:59, Javan Makhmali ***@***.***> wrote: I would expect config.webpacker.caching to cache compiled assets or something. I wonder if we should just use config.cache_classes from Rails to determine this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

javan · 2017-05-10T19:16:53Z

I'd prefer something more descriptive then. Maybe config.webpacker.cache_file_loaders

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

And, generally, it's confusing that we'll have one environment-specific config option in config/environments/*.rb and the rest in config/webpacker/*.

javan · 2017-05-10T19:25:23Z

WDYT about combining config/webpack/paths.yml + config/webpack/development.server.yml into a single file (maybe config/webpack/settings.yml) that also includes this new caching config?

gauravtiwari · 2017-05-10T19:33:41Z

@javan I started doing that in #264. I guess having one setting file makes sense and gives a lot more flexibility towards what we want to configure without worrying about where it should go. We can namespace settings as needed.

gauravtiwari · 2017-05-15T11:13:11Z

@javan @dhh Consolidated the configuration into one file ^_^ Please take a look when you get a chance 👍

javan · 2017-05-18T01:03:40Z

README.md

+your code is most likely to change on each run, manifest is reloaded
+to ensure that latest change and packs has been picked up.
+If you want to toggle this behavior in certain
+environments you can do it in your `config/webpack/configuration.yml`


I don't think "cache" is the best word to describe this behavior. Perhaps invert it with some form or "reload"? Also, should we apply this behavior more broadly to all file loaders and name it as such? Seems like we'd want configuration.yml to reload in dev mode too.

javan · 2017-05-18T01:06:17Z

Since we're down to a single .yml file now, what do you think about moving it to config/webpack.yml or config/webpacker.yml? That way config/webpack/ will just be .js files, which feels nice.

dhh · 2017-05-18T09:39:56Z

👍

…

On May 18, 2017, at 03:06, Javan Makhmali ***@***.***> wrote: Since we're down to a single .yml file now, what do you think about moving it to config/webpack.yml or config/webpacker.yml? That way config/webpack/ will just be .js files, which feels nice. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

gauravtiwari requested a review from guilleiguaran April 26, 2017 19:02

guilleiguaran reviewed Apr 26, 2017

View reviewed changes

rafaelfranca requested changes Apr 27, 2017

View reviewed changes

dhh reviewed Apr 27, 2017

View reviewed changes

rafaelfranca reviewed Apr 27, 2017

View reviewed changes

dhh reviewed May 1, 2017

View reviewed changes

gauravtiwari referenced this pull request May 3, 2017

Use a separate env file to define NODE_ENV

116e038

Remove redundant assign Fix comment Fix sentence

This was referenced May 7, 2017

Add a test helper #341

Closed

Running tests doesn't set NODE_ENV=test #353

Closed

Lazy compile in test env #360

Merged

gauravtiwari added 5 commits May 15, 2017 19:13

Reload config and manifest in both test and development

b9ca626

Move exist? helper method to file loader

c1e88cd

Update changelog

4050e50

Setup webpacker namespace and cache option

28e4cb2

Add usage example for exist? helper method

b1028c8

gauravtiwari added 18 commits May 15, 2017 19:13

Change to caching

852e624

Add a reloadable? method to file loader

18cb314

Update changelog

23dd9d9

Remove reloadable?

89984c3

Explain caching in readme

78857d6

Nitpicking

c3fb9a1

Ignore test app logs

a030df6

Update to rails 5.1.x

3f5b11c

Add test for caching

4bb68e1

Fix quotes

555a3da

Nix exist? and update readme

e4a49bc

Consolidate .yml files into one file and update configuration

745e209

Cleanup paths.yml and fix tests

8ab95b3

Remove extraneous dev server bin var

a995963

Remove old code

9d37ef6

Revert gemfile change

89f7905

Add dev_server helper method

c1c693e

Only cache_manifest

7b2fbae

gauravtiwari changed the title ~~Reload manifest and configuration in both test and development~~ Consolidate configuration into one file and setup caching for manifest May 16, 2017

javan reviewed May 18, 2017

View reviewed changes

gauravtiwari closed this May 20, 2017

gauravtiwari deleted the reload-manifest branch May 20, 2017 11:57

		@@ -1,4 +1,7 @@
		module Webpacker
		mattr_accessor :reload_webpacker

Consolidate configuration into one file and setup caching for manifest #292

Consolidate configuration into one file and setup caching for manifest #292

Uh oh!

Conversation

gauravtiwari commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reload Webpacker manifest when cache_manifest is set

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gauravtiwari commented Apr 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhh commented May 1, 2017

Uh oh!

gauravtiwari commented May 1, 2017

Uh oh!

gauravtiwari commented May 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gauravtiwari May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhh commented May 1, 2017 via email

Uh oh!

gauravtiwari commented May 10, 2017

Uh oh!

javan commented May 10, 2017

Uh oh!

gauravtiwari commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javan commented May 10, 2017

Uh oh!

dhh commented May 10, 2017 via email

Uh oh!

javan commented May 10, 2017

Uh oh!

javan commented May 10, 2017

Uh oh!

javan commented May 10, 2017

Uh oh!

gauravtiwari commented May 10, 2017

Uh oh!

gauravtiwari commented May 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javan commented May 18, 2017

Uh oh!

dhh commented May 18, 2017 via email

Uh oh!

Uh oh!

gauravtiwari commented Apr 26, 2017 •

edited

Loading

Reload Webpacker manifest when `cache_manifest` is set

gauravtiwari May 1, 2017 •

edited

Loading

gauravtiwari commented May 10, 2017 •

edited

Loading