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

Variants in ActionView::Digestor #14243

Merged
merged 1 commit into from Mar 4, 2014
Merged

Variants in ActionView::Digestor #14243

merged 1 commit into from Mar 4, 2014

Conversation

@pch
Copy link
Contributor

pch commented Mar 1, 2014

Take variants into account when calculating template digests in ActionView::Digestor

Fixes #14242

@lukaszx0
Copy link
Member

lukaszx0 commented Mar 1, 2014

@lukaszx0 lukaszx0 self-assigned this Mar 1, 2014
@jeremy
Copy link
Member

jeremy commented Mar 1, 2014

Implementation & test look good to me.

It breaks AV::Digestor backward compatibility without deprecation, though, by introducing a new argument that changes the method signature. Using an args list that may grow in the future is brittle since it'll continue to break compatibility each time it's changed.

@dhh
Copy link
Member

dhh commented Mar 1, 2014

That was my concern too, but Digestor this kinda is a breaking change anyway. If someone is using variants, the digestor is broken if not configured to deal with it. So this seems like a better way to fail early.

On Mar 1, 2014, at 20:25, Jeremy Kemper notifications@github.com wrote:

Implementation & test look good to me.

It breaks AV::Digestor backward compatibility without deprecation, though, by introducing a new argument that changes the method signature. Using an args list that may grow in the future is brittle since it'll continue to break compatibility each time it's changed.


Reply to this email directly or view it on GitHub.

@lukaszx0
Copy link
Member

lukaszx0 commented Mar 1, 2014

Good point, haven't thought about it to be honest.

What about changing format argument into format_or_options, having
deprecation warning with support for the old API while making smooth
transition to new API which will accept options hash instead. We would kill
it in 5.0 release.

What do you think?

On Saturday, March 1, 2014, David Heinemeier Hansson <
notifications@github.com> wrote:

That was my concern too, but Digestor this kinda is a breaking change
anyway. If someone is using variants, the digestor is broken if not
configured to deal with it. So this seems like a better way to fail early.

On Mar 1, 2014, at 20:25, Jeremy Kemper <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

Implementation & test look good to me.

It breaks AV::Digestor backward compatibility without deprecation,
though, by introducing a new argument that changes the method signature.
Using an args list that may grow in the future is brittle since it'll
continue to break compatibility each time it's changed.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14243#issuecomment-36434562
.

@dhh
Copy link
Member

dhh commented Mar 1, 2014

IMO it sucks to have a breaking change, but IMO this isn’t something most people extended in their own app, and even if they did, the variant feature will simply be silently broken unless this is changed. I don’t like breaking shit more than anyone, but imo it’s preferable to break stuff loud and clear, than to have silently corrupt digests that’ll be a bitch to debug (took me a while to find the problem on Basecamp!).

On Mar 1, 2014, at 21:04, Łukasz Strzałkowski notifications@github.com wrote:

Good point, haven't thought about it to be honest.

What about changing format argument into format_or_options, having
deprecation warning with support for the old API while making smooth
transition to new API which will accept options hash instead. We would kill
it in 5.0 release.

What do you think?

On Saturday, March 1, 2014, David Heinemeier Hansson <
notifications@github.com> wrote:

That was my concern too, but Digestor this kinda is a breaking change
anyway. If someone is using variants, the digestor is broken if not
configured to deal with it. So this seems like a better way to fail early.

On Mar 1, 2014, at 20:25, Jeremy Kemper <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

Implementation & test look good to me.

It breaks AV::Digestor backward compatibility without deprecation,
though, by introducing a new argument that changes the method signature.
Using an args list that may grow in the future is brittle since it'll
continue to break compatibility each time it's changed.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14243#issuecomment-36434562
.


Reply to this email directly or view it on GitHub.

@lukaszx0
Copy link
Member

lukaszx0 commented Mar 1, 2014

Completely agree. What I was thinking about was not to break things here
and support both APIs for next 4.x releases making smooth transition to
options hash and having flexible API which will allow us adding new keys to
hash without breaking compatibility. We are doing such things all the time
for every release when we're changing API.

I'm away from computer at the moment. I'll work on this tomorrow and show
you with the code what I mean.

On Saturday, March 1, 2014, David Heinemeier Hansson <
notifications@github.com> wrote:

IMO it sucks to have a breaking change, but IMO this isn’t something most
people extended in their own app, and even if they did, the variant feature
will simply be silently broken unless this is changed. I don’t like
breaking shit more than anyone, but imo it’s preferable to break stuff loud
and clear, than to have silently corrupt digests that’ll be a bitch to
debug (took me a while to find the problem on Basecamp!).

On Mar 1, 2014, at 21:04, Łukasz Strzałkowski <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

Good point, haven't thought about it to be honest.

What about changing format argument into format_or_options, having
deprecation warning with support for the old API while making smooth
transition to new API which will accept options hash instead. We would
kill
it in 5.0 release.

What do you think?

On Saturday, March 1, 2014, David Heinemeier Hansson <
notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

That was my concern too, but Digestor this kinda is a breaking change
anyway. If someone is using variants, the digestor is broken if not
configured to deal with it. So this seems like a better way to fail
early.

On Mar 1, 2014, at 20:25, Jeremy Kemper <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');
<javascript:_e(%7B%7D,'cvml','notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');');>>

wrote:

Implementation & test look good to me.

It breaks AV::Digestor backward compatibility without deprecation,
though, by introducing a new argument that changes the method
signature.
Using an args list that may grow in the future is brittle since it'll
continue to break compatibility each time it's changed.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub<
https://github.com/rails/rails/pull/14243#issuecomment-36434562>
.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14243#issuecomment-36435161
.

@pch
Copy link
Contributor Author

pch commented Mar 1, 2014

def digest(name, format, finder, options = {})

We could also put :variant in the existing options without breaking anything (a bit more messy though). But I agree with @dhh on this.

@jeremy
Copy link
Member

jeremy commented Mar 1, 2014

Def agree that it should be fixed.

However, can it be fixed in a way that won't require a breaking change next time the method signature the needs to change?

Good sign we should be passing an a template "descriptor" instead, like a hash of name: .., format: .., etc, or an array of [name, format, ...].

Also, adding variant will blow all fragment caches in apps that don't use variants yet. Can we preserve the same cache key when variant is nil?

@pch
Copy link
Contributor Author

pch commented Mar 1, 2014

Alternative implementation using a 'descriptor' array instead of format: https://github.com/pch/rails/commit/ef7f838acf2c74eff0c50c2f345fa8d9288c9c8d

wdyt?

Nil variants are ignored, so existing fragment caches should be preserved.

@dhh
Copy link
Member

dhh commented Mar 2, 2014

What I don’t like is if whatever we do doesn’t fail fast when variants are used. Changing the API signature is obviously fail fast, but that also hits people who aren’t using variants. Preferably we’d only fail fast when variants are used. But if the choice is between silent failure and fail fast even when variants are not used, I’d pick the latter.

On Mar 2, 2014, at 0:36, Piotr Chmolowski notifications@github.com wrote:

Alternative implementation using a 'descriptor' array instead of format: pch@ef7f838

wdyt?

Nil variants are ignored, so existing fragment caches should be preserved.


Reply to this email directly or view it on GitHub.

@robin850 robin850 added the actionview label Mar 2, 2014
@lukaszx0 lukaszx0 added this to the 4.1.0 milestone Mar 3, 2014
@lukaszx0
Copy link
Member

lukaszx0 commented Mar 3, 2014

We can also do something like this:

def digest(*args)
  options = {}
  unless args.first.is_a?(Hash)
    ActiveSupport::Deprecation.warn("...")
    options = {
      name: args.first,
      format: args.second,
      finder: args.third,
    }.merge(args.fourth || {})
  else
    options = args.first
  end
  # ...
end

Everybody will be happy. Old API will still work, we'll transition to more flexible, hash based one, for the future and kill deprecated one in 5.x release or 4.2.

@dhh
Copy link
Member

dhh commented Mar 3, 2014

Yeah, it would be nice if people who aren’t using variants can just keep on trucking. Just want to make sure that we are loud about it in the logs that whatever they’re doing will fail with variants if they don’t change. So let’s try this!

On Mar 3, 2014, at 2:45 PM, Łukasz Strzałkowski notifications@github.com wrote:

We can also do something like this:

def digest(*args)
options = {}
unless args.first.is_a?(Hash)
ActiveSupport::Deprecation.warn("...")
options = {
name: args.first,
format: args.second,
finder: args.third,
}.merge(args.fourth || {})
else
options = args.first
end

...

end
Everybody will be happy. Old API will still work, we'll transition to more flexible, hash based one, for the future and kill deprecated one in 5.x release or 4.2.


Reply to this email directly or view it on GitHub.

@pch
Copy link
Contributor Author

pch commented Mar 3, 2014

I've updated the code, implementing your suggestions. Please let me know what you think.

@dhh
dhh reviewed Mar 3, 2014
View changes
actionview/lib/action_view/digestor.rb Outdated
@@ -9,23 +9,48 @@ class Digestor
@@digest_monitor = Monitor.new

class << self
def digest(name, format, finder, options = {})
def digest(*args)

This comment has been minimized.

Copy link
@dhh

dhh Mar 3, 2014

Member

We should add a documentation section describing the options here.

@dhh
Copy link
Member

dhh commented Mar 3, 2014

Code looking good to me. Any comments, @jeremy? Just need a little documentation.

@lukaszx0
Copy link
Member

lukaszx0 commented Mar 3, 2014

When do we plan to remove support for old API? 4.2 or 5.0? I think it would be worth to put this information in both changelog notes and deprecation warning message.

@dhh
Copy link
Member

dhh commented Mar 3, 2014

I'd say 5.0.

@dhh
Copy link
Member

dhh commented Mar 4, 2014

Travis says the build failed. Have you run all the tests with this in place? Eager to merge.

@lukaszx0
Copy link
Member

lukaszx0 commented Mar 4, 2014

Fleaky memcached test - connection failure. Let's wait for rebuild which I've just triggered, should be green.

@pch
Copy link
Contributor Author

pch commented Mar 4, 2014

Sorry, didn't notice. Everything seemed fine locally - will look into it.

@dhh
Copy link
Member

dhh commented Mar 4, 2014

Ah, cools. I’ll merge, then. Thanks again for working on this!

On Mar 4, 2014, at 2:44 PM, Piotr Chmolowski notifications@github.com wrote:

Sorry, didn't notice. Everything seemed fine locally - will look into it.


Reply to this email directly or view it on GitHub.

@lukaszx0
Copy link
Member

lukaszx0 commented Mar 4, 2014

Good to go, it's green again, @pch could you please rebase your branch?

Take variants into account when calculating template digests in
ActionView::Digest.

Digestor#digest now takes a hash as an argument to support variants and
allow more flexibility in the future. Old-style arguments have been
deprecated.

Fixes #14242
@pch
Copy link
Contributor Author

pch commented Mar 4, 2014

Done.

dhh added a commit that referenced this pull request Mar 4, 2014
Variants in ActionView::Digestor
@dhh dhh merged commit 705915a into rails:master Mar 4, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@dhh
Copy link
Member

dhh commented Mar 5, 2014

Hmm, looks like something is still busted.

@template ||= finder.find(logical_name, [], partial?, formats: [ format ], variants: [ variant ])

doesn't actually seem to return the right template. It ignores the variants. Did you guys look at LookupContext to ensure that this would work?

@dhh
Copy link
Member

dhh commented Mar 5, 2014

Problem is that when I ask for a variant of a template that also has a non-variant form, I get the non-variant form back.

@dhh
Copy link
Member

dhh commented Mar 5, 2014

ActionView::ViewPaths doesn't seem to know anything about variants. And we're also not recording a #rendered_variant in the LookupContext. The PartialRenderer also doesn't seem to be aware of variants.

@lukaszx0
Copy link
Member

lukaszx0 commented Mar 5, 2014

Thanks for heads up. I'll work on this today.

On Wednesday, March 5, 2014, David Heinemeier Hansson <
notifications@github.com> wrote:

ActionView::ViewPaths doesn't seem to know anything about variants. And
we're also not recording a #rendered_variant in the LookupContext. The
PartialRenderer also doesn't seem to be aware of variants.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14243#issuecomment-36732159
.

@dhh
Copy link
Member

dhh commented Mar 5, 2014

irb(main):006:0> ApplicationController.new.lookup_context.find "messages/show", [], false, formats: [ :html ], variants: [ :phone ]
=> app/views/desktop/messages/show.html.erb

Even though I have app/views/phone/messages/show.html+phone.erb available and that's been added with prepend_view_path Rails.root.join("app/views/phone")

@pch
Copy link
Contributor Author

pch commented Mar 8, 2014

@dhh could you paste you view paths here? So far, I've only managed to figure out that the following order seems to pick both desktop & phone versions:

prepend_view_path Rails.root.join("app/views/desktop")
prepend_view_path Rails.root.join("app/views/phone")

but when you prepend phone first, it'll only look for views in desktop.

@pch
Copy link
Contributor Author

pch commented Mar 8, 2014

More examples:

views/
  desktop/
    messages/
      show.html.erb

  phone/
    messages/
      show.html+phone.erb
prepend_view_path Rails.root.join("app/views/phone")
prepend_view_path Rails.root.join("app/views/desktop")

The desktop folder will be picked first with the following pattern:

Dir["/Users/piotrek/test-app/app/views/desktop/messages/show{.en,}{.html,}{+phone,}{.erb,.builder,.raw,.ruby,.jbuilder,.coffee,}"]
#=> ["/Users/piotrek/test-app/app/views/desktop/messages/show.html.erb"]

Variant is optional here, so the next directory in line (phone) won't be traversed, since PathSet settles on the first thing it finds.

(If I understand correctly)

@dhh
Copy link
Member

dhh commented Mar 8, 2014

Hmm. Preferably order shouldn’t matter. They should search all of these view paths before selecting. Not just the first good enough match.

On Mar 8, 2014, at 5:59 PM, Piotr Chmolowski notifications@github.com wrote:

More examples:

views
desktop
messages
show.html.erb

phone
messages
show.html+phone.erb

prepend_view_path Rails.root.join("app/views/phone")
prepend_view_path Rails.root.join("app/views/desktop")

The desktop folder will be picked first with the following pattern:

Dir["/Users/piotrek/test-app/app/views/desktop/home/index{.en,}{.html,}{+phone,}{.erb,.builder,.raw,.ruby,.jbuilder,.coffee,}""]
#=> ["/Users/piotrek/test-app/app/views/desktop/home/index.html.erb"]
Variant is optional here, so the next directory in line (phone) won't be traversed, since PathResolver settles on the first thing it finds.


Reply to this email directly or view it on GitHub.

@pch
Copy link
Contributor Author

pch commented Mar 8, 2014

It seems to be the case for language versions as well:

desktop/
  messages/
    show.html.erb
phone/
  messages/
    show.en.html.erb
prepend_view_path Rails.root.join("app/views/phone")
prepend_view_path Rails.root.join("app/views/desktop")

In this case phone/messages/show.en.html.erb should have the priority, but desktop/messages/show.html.erb will be selected.

@dhh
Copy link
Member

dhh commented Mar 8, 2014

Yeah, that’s no bueno.

But first things first. Let’s make sure the current path, as it exists for the renderer, is right for digester too.

Then let’s fix the ordering problem.

On Mar 8, 2014, at 9:46 PM, Piotr Chmolowski notifications@github.com wrote:

It seems to be the case for language versions as well:

desktop/
messages/
show.html.erb
phone/
messages/
show.en.html.erb

prepend_view_path Rails.root.join("app/views/phone")
prepend_view_path Rails.root.join("app/views/desktop")
In this case phone/messages/show.en.html.erb should have the priority, but desktop/messages/show.html.erb will be selected.


Reply to this email directly or view it on GitHub.

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

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.