Fix generators (@dgynn); load Railtie only with Rails, ensures caching configured #1352

bf4 · 2015-11-30T05:48:09Z

Fixing the generators (@dgynn)

Only load generators when needed

use hook_for to hook in the serializer and remove load_generators
move generators so they can be found by rails
move to_prepare block to railtie config

This commit improves the way the generators are loaded and how
they extend the resource generator.

The initializer block has been changed to a generator block which is only executed when generators are needed.
The call to app.load_generators has been removed. There is no need to load all generators.
The resource_override.rb has been changed to use hook_for to extend the resource generator.
The directory for the generators has been moved to match the way Rails looks to load generators.

With hook_for it would now be possible for a user to pass --no-serializer to skip that option.
The --serialize option also now shows up in the generator help with rails g resource --help.

These changes follow the way the Draper gem extends the controller generator.

Putting Rails Code in the Railties (@bf4)

akak let people use AMS without requiring 'rails' or 'action{pack,view}'

Only call railtie when Rails is defined; assume controller loaded

Isolated Testing

Rake test inspired by https://github.com/rails/rails/blob/v5.0.0.beta1/activejob/Rakefile
Isolated unit inspired by
- https://github.com/rails/rails/blob/v5.0.0.beta1/railties/test/isolation/abstract_unit.rb
- https://github.com/rails/rails/blob/v5.0.0.beta1/activemodel/test/cases/railtie_test.rb

Misc

Turns out mattr_accessor(:logger) { ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDOUT)) }
was always nil until the Railtie was loaded, since mattr_accessor
block defaults don't really work on modules, but on the classes that
include them.
Should resolve 0.10.0.rc3 outside of rails? #1389 where Railties was being too-eagerly loaded.

Commented on importance of Rails being required first for caching to
work. Ref Cache not working #923 (comment)

+    # To be useful, this hook must run after Rails has initialized,
+    # BUT before any serializers are loaded.
+    # Otherwise, the call to 'cache' won't find `cache_store` or `perform_caching`
+    # defined, and serializer's `_cache_store` will be nil.
+    # IF the load order cannot be changed, then in each serializer that that defines a `cache`,
+    # manually specify e.g. `PostSerializer._cache_store = Rails.cache` any time
+    # before the serializer is used.  (Even though `ActiveModel::Serializer._cache_store` is
+    # inheritable, we don't want to set it on `ActiveModel::Serializer` directly unless
+    # we want *every* serializer to be considered cacheable, regardless of specifying
+    # `cache # some options` in a serializer or not.
+    initializer 'active_model_serializers.caching' => :after_initialize do
+      ActiveModelSerializers.config.cache_store     = ActionController::Base.cache_store
+      ActiveModelSerializers.config.perform_caching = Rails.configuration.action_controller.perform_caching
+    end

In isolated tests, active_support/core_ext/object/with_options is required.

bf4 · 2015-11-30T15:46:58Z

lib/active_model_serializers/railtie.rb

+          ActiveModel::Serializer.serializers_cache.clear
+        end
+      end
+    end


I wonder if maybe this shouldn't be in an initializer

It does, however, make it easier to conditionally disable including the module into action controller. e.g. https://github.com/rails-api/active_model_serializers/pull/592/files and #1295 (comment)

Having the include ::ActionController::Serialization in an initializer is good for being able to conditionally disable it based on whatever gets decided from #1295. And having the on_load(:action_controller) is good for lazy-loading in case ActionController::Base is never loaded. Those both seem good.

The to_prepare block does not need to be in the initializer. It could be outside the initializers on the Railtie itself with just...

config.to_prepare do ActiveModel::Serializer.serializers_cache.clear end

Using an initializer, and a load hook (:action_controller), and then running another load hook (:active_model_serializers) to set the logger seems like overly complicated. Depending on load ordering, someone could have an initializer that explicitly sets ActiveModelSerializers.logger but then gets overwritten.
Is there any other reason to have the :active_model_serializers lazy-load hook? Could that logic just get folded in where the run_load_hooks call is? Would you ever expect someone else to hook into that? And if so, would it only be after :action_controller has been loaded?

I'm still poking around a bit to see if I've got the order right on some of these.

@dgynn awesome! Thanks for looking at this! Feel free to submit a PR into it if you'd like.

Also, see #1353 which is similar to #1295

@dgynn

Using an initializer, and a load hook (:action_controller), and then running another load hook (:active_model_serializers) to set the logger seems like overly complicated. Depending on load ordering, someone could have an initializer that explicitly sets ActiveModelSerializers.logger but then gets overwritten.
Is there any other reason to have the :active_model_serializers lazy-load hook?

Since the logger isn't necessarily derived from the controller, I'm just being paranoid that someone might configure the logger before the controller was loaded. This makes sure the user has the final say.. but I could me misthinking that

bf4 · 2015-12-02T05:10:51Z

cc @rails-api/ams

bf4 · 2015-12-22T16:42:30Z

ref: @dgynn has made a PR into the merge source bf4#1

bf4 · 2015-12-23T05:52:42Z

@dgynn merged in bf4#1

bf4 · 2015-12-25T09:29:31Z

Rakefile

+      sh(Gem.ruby, '-w', "-I#{dir}/lib", "-I#{dir}/test", file) or fail 'Failures' # rubocop:disable Style/AndOr
+    end
+  end
+end


see commit message, but basically this is adapted from ActiveJob Rakefile

beauby · 2015-12-29T22:15:00Z

.travis.yml

-  - rvm: jruby-9000
-    env: JRUBY_OPTS='--server -Xcompile.invokedynamic=false -Xcli.debug=true --debug'
+  - rvm: jruby-9.0.4.0
+    env: JRUBY_OPTS='-Xcompat.version=2.0 --server -Xcompile.invokedynamic=false -Xcli.debug=true --debug'


Does this change belong here?

No, from the jruby pr. Had a failute

B mobile phone

On Dec 29, 2015, at 4:15 PM, Lucas Hosseini [email protected] wrote:

In .travis.yml:

@@ -33,8 +33,8 @@ matrix:
include:

rvm: 2.2
env: CAPTURE_STDERR=true

rvm: jruby-9000

env: JRUBY_OPTS='--server -Xcompile.invokedynamic=false -Xcli.debug=true --debug'

rvm: jruby-9.0.4.0

env: JRUBY_OPTS='-Xcompat.version=2.0 --server -Xcompile.invokedynamic=false -Xcli.debug=true --debug'
Does this change belong here?

—
Reply to this email directly or view it on GitHub.

beauby · 2015-12-29T22:16:32Z

I'm not familiar enough with railties. Would you mind summarizing what this PR does?

bf4 · 2015-12-30T04:53:35Z

@beauby Updated the description from the commit messages #1352 (comment). Thanks for the nudge

bf4 · 2016-01-14T02:28:01Z

Rakefile

+end
+
+if ENV['RAILS_VERSION'].to_s > '4.0' && RUBY_ENGINE == 'ruby'
+  task default: [:isolated, :test, :rubocop]


isolated tests as set up here break on 4.0 and on JRuby

dgynn · 2016-01-14T06:47:39Z

I've pushed up a PR to your branch that shows the changes I'd suggest. I did not update all the tests or change the comment about cache setup. Feel free to edit or disregard. :)

BTW, you might be interested in a tool I'm working on that inspects Rails configuration info.
https://github.com/dgynn/tuttle
In particular, it can list out all of the initializers configured for an app and show their order and dependencies. Here is a demo of that with my suggested changes installed. Look at the initializers tab...
https://tuttle-demo.herokuapp.com/tuttle/rails

Working on that is how I came across the generators issue initially.

maurogeorge · 2016-01-14T21:31:31Z

lib/active_model_serializers.rb

-require 'active_model/serializable_resource'
-require 'active_model/serializer/version'
+  class << self; attr_accessor :logger; end
+  self.logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDOUT))


I think the mattr_accessor(:logger) { ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDOUT)) } is more legible than this one.

I believe mattr_accessor does not support blocks in ActiveSupport 4.0.

See pr description. It didn't work

- use hook_for to hook in the serializer and remove load_generators - move generators so they can be found by rails - move to_prepare block to railtie config This commit improves the way the generators are loaded and how they extend the resource generator. * The initializer block has been changed to a `generator` block which is only executed when generators are needed. * The call to `app.load_generators` has been removed. There is no need to load *all* generators. * The `resource_override.rb` has been changed to use `hook_for` to extend the resource generator. * The directory for the generators has been moved to match the way Rails looks to load generators. With `hook_for` it would now be possible for a user to pass `--no-serializer` to skip that option. The `--serialize` option also now shows up in the generator help with `rails g resource --help`. These changes follow the way the Draper gem extends the `controller` generator.

Isolated Testing - Rake test inspired by https://github.com/rails/rails/blob/v5.0.0.beta1/activejob/Rakefile - Isolated unit inspired by - https://github.com/rails/rails/blob/v5.0.0.beta1/railties/test/isolation/abstract_unit.rb - https://github.com/rails/rails/blob/v5.0.0.beta1/activemodel/test/cases/railtie_test.rb Misc - Turns out `mattr_accessor(:logger) { ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDOUT)) }` was always nil until the Railtie was loaded, since mattr_accessor block defaults don't really work on modules, but on the classes that include them. - Commented on important on Rails being required first for caching to work. - In isolated tests, `active_support/core_ext/object/with_options` is required.

this uses the configuration settings rather than calling ActionController::Base to get the configured values. after the "action_controller.set_configs" initializer has run, the configuration option holds the value Base will get when it loads.

bf4 · 2016-01-15T07:58:47Z

@maurogeorge @dgynn updated

maurogeorge · 2016-01-15T09:30:27Z

LGTM. I only recommend take a look at the @dgynn comments #1352 (comment) #1352 (comment) that worked on this part of the project 👍

bf4 · 2016-01-15T09:31:55Z

@maurogeorge I merged in his code... but I'm tired.. besides the value in re-reading the comments, was there more work to do that I missed?

maurogeorge · 2016-01-15T09:39:31Z

@bf4 no, to me looks good. @dgynn let us know if you have more concerns about this.

dgynn · 2016-01-15T17:33:57Z

This looks good. I do think the comment in the Railtie about caching can now be removed but that should be verified.
Also, the 2 initializer blocks for caching and logging could be combined. There really isn't much benefit to having them separate. ActionController itself used to have 2 separate initializers for logging and caching and they combined them here... rails/rails@fbc9d0f

bf4 · 2016-01-15T17:56:23Z

Can I give you commit to my fork to finish it? I'm on vacation

Re comment there are tests for it

B mobile phone

On Jan 15, 2016, at 12:34 PM, Dave Gynn [email protected] wrote:

This looks good. I do think the comment in the Railtie about caching can now be removed but that should be verified.
Also, the 2 initializer blocks for caching and logging could be combined. There really isn't much benefit to having them separate. ActionController itself used to have 2 separate initializers for logging and caching and they combined them here... rails/rails@fbc9d0f

—
Reply to this email directly or view it on GitHub.

this also changes the action_controller load hook to not trigger loading of the ActionController::Base

dgynn · 2016-01-16T22:22:53Z

I've updated the Railtie to combine the initializers and update the comment. I believe this initializer ordering should resolve the caching issue from #923 and also not trigger too-early loading of ActionController::Base. I'd say this is ready to be merged.

bf4 · 2016-01-17T04:17:22Z

@dgynn looks good. pr description still good summary?

bf4 · 2016-01-17T04:20:12Z

lib/active_model_serializers/railtie.rb

@@ -0,0 +1,36 @@
+require 'rails/railtie'
+require 'action_controller'
+require 'action_controller/railtie'


This still necessary?

The AMS railtie does require that the action_controller/railtie has been loaded so that Rails.configuration.action_controller exists. But there isn't any normal that wouldn't have already been required. It should have been loaded by rails/all or even active_record/railtie (see the comment here https://github.com/rails/rails/blob/master/activerecord/lib/active_record/railtie.rb#L5).

bf4 · 2016-01-19T00:47:10Z

Ok, anything than needs to be resolved before merging this? I'll add an issue with label 'pr followup' for what we've discussed

dgynn · 2016-01-19T01:33:10Z

@bf4 This looks good to merge to me.

@dgynn

Fix generators (@dgynn); load Railtie only with Rails, ensures caching configured

bf4 · 2016-01-19T04:24:57Z

merged. Let the heavens be glad; let the earth rejoice

bf4 force-pushed the railties branch from bf1fa90 to b8d460c Compare November 30, 2015 06:26

bf4 reviewed Nov 30, 2015
View reviewed changes

bf4 added V: 0.10.x C: Cleanup labels Nov 30, 2015

bf4 mentioned this pull request Dec 13, 2015

Only load generators when needed #1374

Closed

bf4 force-pushed the railties branch from b8d460c to 31f58f2 Compare December 23, 2015 05:36

bf4 mentioned this pull request Dec 23, 2015

Prevent loading of all generators in initializer bf4/active_model_serializers#1

Closed

bf4 force-pushed the railties branch from 8f99a9e to 6ca5966 Compare December 25, 2015 09:28

bf4 reviewed Dec 25, 2015
View reviewed changes

bf4 force-pushed the railties branch 2 times, most recently from b63f6ca to a7e16ad Compare December 28, 2015 05:42

beauby reviewed Dec 29, 2015
View reviewed changes

bf4 force-pushed the railties branch from a7e16ad to 31dba53 Compare December 30, 2015 04:43

bf4 force-pushed the railties branch from 31dba53 to 5fb5869 Compare December 30, 2015 04:59

bf4 mentioned this pull request Jan 4, 2016

cache not working #1210

Closed

bf4 force-pushed the railties branch from 5fb5869 to 3d0826e Compare January 4, 2016 05:20

bf4 force-pushed the railties branch 5 times, most recently from de0189c to a2f30a1 Compare January 14, 2016 02:26

bf4 reviewed Jan 14, 2016
View reviewed changes

maurogeorge reviewed Jan 14, 2016
View reviewed changes

bf4 and others added 7 commits January 15, 2016 01:52

Collect more Rails initialization code in the Railtie

0c2153a

Fix load-order issues

fe015d1

Add CHANGELOG

58a74d0

Replace load hook :active_model_serializers with :action_controller

d3bdc9b

bf4 force-pushed the railties branch from 8a9b712 to d3bdc9b Compare January 15, 2016 07:58

combine config initializers and update comments

6713864

this also changes the action_controller load hook to not trigger loading of the ActionController::Base

bf4 reviewed Jan 17, 2016
View reviewed changes

bf4 added a commit that referenced this pull request Jan 19, 2016

Merge pull request #1352 from bf4/railties

8981683

Fix generators (@dgynn); load Railtie only with Rails, ensures caching configured

bf4 merged commit 8981683 into rails-api:master Jan 19, 2016

bf4 deleted the railties branch January 19, 2016 04:24

bf4 added the S: Needs followup label Feb 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix generators (@dgynn); load Railtie only with Rails, ensures caching configured #1352

Fix generators (@dgynn); load Railtie only with Rails, ensures caching configured #1352

bf4 commented Nov 30, 2015

bf4 Nov 30, 2015

bf4 Nov 30, 2015

dgynn Dec 16, 2015

bf4 Dec 16, 2015

bf4 Dec 16, 2015

bf4 commented Dec 2, 2015

bf4 commented Dec 22, 2015

bf4 commented Dec 23, 2015

bf4 Dec 25, 2015

beauby Dec 29, 2015

bf4 Dec 29, 2015

beauby commented Dec 29, 2015

bf4 commented Dec 30, 2015

bf4 Jan 14, 2016

dgynn commented Jan 14, 2016

maurogeorge Jan 14, 2016

dgynn Jan 14, 2016

bf4 Jan 17, 2016

maurogeorge Jan 18, 2016

bf4 commented Jan 15, 2016

maurogeorge commented Jan 15, 2016

bf4 commented Jan 15, 2016

maurogeorge commented Jan 15, 2016

dgynn commented Jan 15, 2016

bf4 commented Jan 15, 2016

dgynn commented Jan 16, 2016

bf4 commented Jan 17, 2016

bf4 Jan 17, 2016

dgynn Jan 17, 2016

bf4 commented Jan 19, 2016

dgynn commented Jan 19, 2016

bf4 commented Jan 19, 2016

Fix generators (@dgynn); load Railtie only with Rails, ensures caching configured #1352

Fix generators (@dgynn); load Railtie only with Rails, ensures caching configured #1352

Conversation

bf4 commented Nov 30, 2015

Fixing the generators (@dgynn)

Putting Rails Code in the Railties (@bf4)

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

bf4 commented Dec 2, 2015

bf4 commented Dec 22, 2015

bf4 commented Dec 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Dec 29, 2015

bf4 commented Dec 30, 2015

Choose a reason for hiding this comment

dgynn commented Jan 14, 2016

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

bf4 commented Jan 15, 2016

maurogeorge commented Jan 15, 2016

bf4 commented Jan 15, 2016

maurogeorge commented Jan 15, 2016

dgynn commented Jan 15, 2016

bf4 commented Jan 15, 2016

dgynn commented Jan 16, 2016

bf4 commented Jan 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Jan 19, 2016

dgynn commented Jan 19, 2016

bf4 commented Jan 19, 2016