Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add wildcard template dependencies. #20904

Merged
merged 1 commit into from Jul 27, 2015

Conversation

kaspth
Copy link
Contributor

@kaspth kaspth commented Jul 16, 2015

Fixes #20878.

@kaspth kaspth self-assigned this Jul 16, 2015
@kaspth kaspth added this to the 5.0.0 milestone Jul 16, 2015
@@ -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|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 kaspth force-pushed the wildcard-template-dependencies branch from c479e28 to 2812436 Compare July 22, 2015 20:25
teardown :teardown_app

test "dependency tracker view_paths are set in initializer" do
assert_equal ActionController::Base.view_paths, ActionView::DependencyTracker.view_paths
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best way I could find to test initializer behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the best way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails way is best way™️

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

kaspth commented Jul 22, 2015

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

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

dhh commented Jul 23, 2015

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 kaspth force-pushed the wildcard-template-dependencies branch 2 times, most recently from b6275bf to f8563c9 Compare July 23, 2015 19:28
@kaspth
Copy link
Contributor Author

kaspth commented Jul 23, 2015

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


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

mattr_accessor(:view_paths) { ActionView::PathSet.new }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it need its own view paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 kaspth force-pushed the wildcard-template-dependencies branch from f8563c9 to ae703ec Compare July 24, 2015 17:17
@kaspth
Copy link
Contributor Author

kaspth commented Jul 24, 2015

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
Copy link
Contributor Author

kaspth commented Jul 24, 2015

🏃 Travis 🏃

@kaspth kaspth closed this Jul 24, 2015
@kaspth kaspth reopened this Jul 24, 2015
@kaspth
Copy link
Contributor Author

kaspth commented Jul 25, 2015

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 👍

@kaspth kaspth force-pushed the wildcard-template-dependencies branch from ae703ec to 591834d Compare July 25, 2015 18:54
@kaspth kaspth closed this Jul 25, 2015
@kaspth kaspth reopened this Jul 25, 2015
@dhh
Copy link
Member

dhh commented Jul 26, 2015

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

@@ -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 # '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wups! you're right.

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

:shipit:

kaspth added a commit that referenced this pull request Jul 27, 2015
@kaspth kaspth merged commit c5583cd into rails:master Jul 27, 2015
@kaspth kaspth deleted the wildcard-template-dependencies branch July 27, 2015 18:27
@kaspth
Copy link
Contributor Author

kaspth commented Jul 27, 2015

Boom! Thanks everyone 🎉

@dhh
Copy link
Member

dhh commented Jul 27, 2015

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

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
@dhh
Copy link
Member

dhh commented Feb 10, 2016

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
Copy link
Contributor Author

kaspth commented Feb 10, 2016

@dhh yup, will do 👍


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if tracker.try(:supports_view_paths?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use try internally :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants