Add wildcard template dependencies. #20904

kaspth · 2015-07-16T20:24:51Z

Fixes #20878.

kaspth · 2015-07-16T20:31:26Z

actionview/lib/action_view/dependency_tracker.rb

@@ -142,8 +142,20 @@ def add_static_dependency(dependencies, dependency)
          end
        end

+        def resolve_directories(wildcard_dependencies)
+          wildcard_dependencies.map do |dependency|
+            Dir[ "digestor/#{dependency}" ].map do |file|


So this is where the hack is. I can't figure out how to reliably find a way to prefix the dependency path with the correct directory nesting.

Some ideas I've been throwing around:

Extend Action View's resolvers to support wildcard lookups (they have the most info about paths and templates and such).

Inject a shared prefix path (perhaps as a threaded mattr_accessor)

Either way, the problem is that the prefix is fixed here:

rails/actionview/test/template/digestor_test.rb

Line 37 in 094571b

FixtureTemplate.new("digestor/#{partial_name}.#{format}.erb")

Which probably doesn't work the same way as it does outside of tests either.

After having looked at this some more, we probably need to give the dependency tracker access to either a view_paths or a lookup_context.
I still don't know enough about either view paths or lookup contexts, to say if this can work out yet. I'll keep investigating.

kaspth · 2015-07-22T20:26:39Z

railties/test/application/wildcard_dependencies_test.rb

+  teardown :teardown_app
+
+  test "dependency tracker view_paths are set in initializer" do
+    assert_equal ActionController::Base.view_paths, ActionView::DependencyTracker.view_paths


Best way I could find to test initializer behavior.

It is the best way.

Rails way is best way:tm:

kaspth · 2015-07-22T20:34:21Z

@dhh I got something that's working here too, but I still need to add documentation.

I don't know how necessary it is to add a cache to the wildcard lookup. I'll try doing some benchmarking.

@jeremy @rafaelfranca the implementation I've done feels, eh, not so nice to me. Can you review? 😁

dhh · 2015-07-23T09:57:36Z

Awesome. I do think we'll need a cache, though. Otherwise every time that
template is eval'ed, you have to touch the filesystem. That won't fly.

On Wed, Jul 22, 2015 at 10:35 PM, Kasper Timm Hansen <
notifications@github.com> wrote:

@dhh https://github.com/dhh I got something that's working here too,
but I still need to add documentation.

I don't know how necessary it is to add a cache to the wildcard lookup.
I'll try doing some benchmarking.

@jeremy https://github.com/jeremy @rafaelfranca
https://github.com/rafaelfranca the implementation I've done feels, eh,
not so nice to me. Can you review? [image: 😁]

—
Reply to this email directly or view it on GitHub
#20904 (comment).

kaspth · 2015-07-23T19:38:59Z

@dhh I've added a changelog entry, documentation and, most importantly, caching 😁

rafaelfranca · 2015-07-23T19:41:23Z

actionview/lib/action_view/dependency_tracker.rb


 module ActionView
  class DependencyTracker # :nodoc:
    @trackers = ThreadSafe::Cache.new

+    mattr_accessor(:view_paths) { ActionView::PathSet.new }


Why does it need its own view paths?

The only place I found that I could steal them from was ActionController::Base's, which I can't depend on here. So this is a fallback.

Do you know of another place I could steal paths from? 😄

Yeah, I can see that. You are trying to avoid coupling with Action Pack. But is views_paths shared across all controllers or can I override it just for a controller? If that is the case this will not work.

Right, I think you can override view_paths. Hm, not sure, how to catch those usages. Can we get a reference to the controller in those cases? If you happen to know that is.

I think the view context has a controller reference, but I think the tracker run before the context is created. But worth to investigate.

Thanks, I’ll take a look.

Den 23/07/2015 kl. 21.57 skrev Rafael Mendonça França notifications@github.com:

In actionview/lib/action_view/dependency_tracker.rb #20904 (comment):

module ActionView
class DependencyTracker # :nodoc:
@trackers = ThreadSafe::Cache.new

mattr_accessor(:view_paths) { ActionView::PathSet.new }
I think the view context has a controller reference, but I think the tracker run before the context is created. But worth to investigate.

—
Reply to this email directly or view it on GitHub https://github.com/rails/rails/pull/20904/files#r35364247.

Kasper

Good call here, @rafaelfranca. The digestor gets the lookup_context injected through the finder argument, so we can get the view_paths from there.

The only problem, is that we have to add a third argument to the tracker's call method, which other trackers definitely don't support yet. Since I don't know of a reliable way to check how many args a method takes, I've added an option trackers should specify when registering.

kaspth · 2015-07-24T17:33:55Z

I'm not sure where we document that custom trackers must support the view_paths argument to be able to use these wildcard dependencies.

kaspth · 2015-07-24T17:34:17Z

🏃 Travis 🏃

kaspth · 2015-07-25T18:45:52Z

I've looked at how Haml and Slim register themselves and they both use the ERB tracker. Perhaps the trackers should implement supports_view_paths instead of having it as a configured that's used when registering, then it'll just work for Haml and Slim and possibly others.

I've talked myself into doing that. I'll change it now 👍

dhh · 2015-07-26T07:05:41Z

Excellent! @rafaelfranca do you have any comments on the view handler implementation?

repinel · 2015-07-26T17:14:13Z

actionview/test/template/digestor_test.rb

@@ -30,7 +32,7 @@ def details_key
  end

  def find(name, prefixes = [], partial = false, keys = [], options = {})
-    partial_name = partial ? name.gsub(%r|/([^/]+)$|, '/_\1') : name
+    partial_name = partial ? name.gsub(%r|/([^/]+)$|, '/_\1') : name # '


We need this comment?

Wups! you're right.

rafaelfranca · 2015-07-27T18:07:34Z

Add wildcard template dependencies.

kaspth · 2015-07-27T18:28:16Z

Boom! Thanks everyone 🎉

dhh · 2015-07-27T19:06:20Z

Awesome work, Kasper!

On Mon, Jul 27, 2015 at 8:29 PM, Kasper Timm Hansen <
notifications@github.com> wrote:

Boom! Thanks everyone [image: 🎉]

—
Reply to this email directly or view it on GitHub
#20904 (comment).

dhh · 2016-02-10T15:30:34Z

Found an issue with this in deployment. The dependency tree generated by the wildcards is not deterministic. I think the directory scan returns results in an unordered fashion. This means each server generates a unique digest for the same wildcard dependency declaration. Which means that the caching doesn't work. I remember we used to have this problem with Sprockets as well. @kaspth can you investigate?

kaspth · 2016-02-10T16:10:17Z

@dhh yup, will do 👍

eduardoschneiders · 2016-02-10T16:30:10Z

actionview/lib/action_view/dependency_tracker.rb


-      if tracker.present?
-        tracker.call(name, template)
+      if tracker.respond_to?(:supports_view_paths?) && tracker.supports_view_paths?


Isn't it better to do something like this here?

if tracker.try(:supports_view_paths?)

We don't use try internally :)

kaspth added the actionview label Jul 16, 2015

kaspth self-assigned this Jul 16, 2015

kaspth added this to the 5.0.0 milestone Jul 16, 2015

kaspth reviewed Jul 16, 2015
View reviewed changes

kaspth force-pushed the wildcard-template-dependencies branch from c479e28 to 2812436 Compare July 22, 2015 20:25

kaspth reviewed Jul 22, 2015
View reviewed changes

kaspth force-pushed the wildcard-template-dependencies branch 2 times, most recently from 0a089c6 to 78a1dae Compare July 22, 2015 20:29

kaspth force-pushed the wildcard-template-dependencies branch from 78a1dae to 38358bb Compare July 22, 2015 20:39

kaspth force-pushed the wildcard-template-dependencies branch 2 times, most recently from b6275bf to f8563c9 Compare July 23, 2015 19:28

rafaelfranca reviewed Jul 23, 2015
View reviewed changes

kaspth force-pushed the wildcard-template-dependencies branch from f8563c9 to ae703ec Compare July 24, 2015 17:17

kaspth closed this Jul 24, 2015

kaspth reopened this Jul 24, 2015

kaspth force-pushed the wildcard-template-dependencies branch from ae703ec to 591834d Compare July 25, 2015 18:54

kaspth closed this Jul 25, 2015

kaspth reopened this Jul 25, 2015

repinel reviewed Jul 26, 2015
View reviewed changes

Add wildcard template dependencies.

a6509d3

kaspth force-pushed the wildcard-template-dependencies branch from 591834d to a6509d3 Compare July 26, 2015 17:21

kaspth added a commit that referenced this pull request Jul 27, 2015

Merge pull request #20904 from kaspth/wildcard-template-dependencies

c5583cd

Add wildcard template dependencies.

kaspth merged commit c5583cd into rails:master Jul 27, 2015

kaspth deleted the wildcard-template-dependencies branch July 27, 2015 18:27

rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

eduardoschneiders reviewed Feb 10, 2016
View reviewed changes

kaspth mentioned this pull request Feb 10, 2016

Action View Digests for wildcard declarations aren't deterministic #23592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wildcard template dependencies. #20904

Add wildcard template dependencies. #20904

Add wildcard template dependencies. #20904

Add wildcard template dependencies. #20904

Conversation

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