Variants in ActionView::Digestor #14243

pch · 2014-03-01T18:03:55Z

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

lukaszx0 · 2014-03-01T18:07:35Z

jeremy · 2014-03-01T19:24:54Z

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 · 2014-03-01T19:51:28Z

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 [email protected] 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 · 2014-03-01T20:03:32Z

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 <
[email protected]> 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 <[email protected]javascript:_e(%7B%7D,'cvml','[email protected]');>
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 · 2014-03-01T20:12:57Z

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 [email protected] 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 <
[email protected]> 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 <[email protected]javascript:_e(%7B%7D,'cvml','[email protected]');>
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 · 2014-03-01T20:20:42Z

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 <
[email protected]> 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 <[email protected]javascript:_e(%7B%7D,'cvml','[email protected]');>
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 <
[email protected]javascript:_e(%7B%7D,'cvml','[email protected]');>
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 <[email protected]javascript:_e(%7B%7D,'cvml','[email protected]');
<javascript:_e(%7B%7D,'cvml','[email protected]javascript:_e(%7B%7D,'cvml','[email protected]');');>>

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 · 2014-03-01T20:33:47Z

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 · 2014-03-01T22:31:19Z

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 · 2014-03-01T23:35:44Z

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 · 2014-03-02T12:20:49Z

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 [email protected] 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.

lukaszx0 · 2014-03-03T13:44:36Z

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 · 2014-03-03T13:47:56Z

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 [email protected] 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 · 2014-03-03T16:03:16Z

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

dhh · 2014-03-03T16:08:47Z

actionview/lib/action_view/digestor.rb

We should add a documentation section describing the options here.

dhh · 2014-03-03T16:09:27Z

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

lukaszx0 · 2014-03-03T16:11:13Z

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 · 2014-03-03T16:11:34Z

I'd say 5.0.

dhh · 2014-03-04T13:39:34Z

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

lukaszx0 · 2014-03-04T13:43:31Z

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

pch · 2014-03-04T13:43:44Z

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

dhh · 2014-03-04T13:45:18Z

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

On Mar 4, 2014, at 2:44 PM, Piotr Chmolowski [email protected] wrote:

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

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

lukaszx0 · 2014-03-04T14:00:14Z

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 · 2014-03-04T14:04:35Z

Done.

Variants in ActionView::Digestor

dhh · 2014-03-05T11:06:54Z

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 · 2014-03-05T11:07:27Z

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 · 2014-03-05T11:13:04Z

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 · 2014-03-05T11:15:09Z

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

On Wednesday, March 5, 2014, David Heinemeier Hansson <
[email protected]> 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 · 2014-03-05T11:20:10Z

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 · 2014-03-08T15:57:09Z

@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 · 2014-03-08T16:58:30Z

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 · 2014-03-08T20:30:17Z

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 [email protected] 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 · 2014-03-08T20:45:54Z

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 · 2014-03-08T20:49:29Z

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 [email protected] 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.

lukaszx0 self-assigned this Mar 1, 2014

robin850 added the actionview label Mar 2, 2014

lukaszx0 added this to the 4.1.0 milestone Mar 3, 2014

dhh reviewed Mar 3, 2014
View reviewed changes

actionview/lib/action_view/digestor.rb Outdated

Copy link

Member

dhh Mar 3, 2014

Choose a reason for hiding this comment

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

We should add a documentation section describing the options here.

Variants in ActionView::Digestor

1858cc6

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

dhh added a commit that referenced this pull request Mar 4, 2014

Merge pull request #14243 from pch/digestor-variants

705915a

Variants in ActionView::Digestor

dhh merged commit 705915a into rails:master Mar 4, 2014

pch mentioned this pull request Mar 6, 2014

Digestor: pass the missing "keys" param to LookupContext #14293

Closed

pch mentioned this pull request Mar 8, 2014

Ensure LookupContext in Digestor selects correct variant #14329

Merged

Variants in ActionView::Digestor #14243

Variants in ActionView::Digestor #14243

Uh oh!

Conversation

pch commented Mar 1, 2014

Uh oh!

lukaszx0 commented Mar 1, 2014

Uh oh!

jeremy commented Mar 1, 2014

Uh oh!

dhh commented Mar 1, 2014

Uh oh!

lukaszx0 commented Mar 1, 2014

Uh oh!

dhh commented Mar 1, 2014

Uh oh!

lukaszx0 commented Mar 1, 2014

Uh oh!

pch commented Mar 1, 2014

Uh oh!

jeremy commented Mar 1, 2014

Uh oh!

pch commented Mar 1, 2014

Uh oh!

dhh commented Mar 2, 2014

Uh oh!

lukaszx0 commented Mar 3, 2014

Uh oh!

dhh commented Mar 3, 2014

...

Uh oh!

pch commented Mar 3, 2014

Uh oh!

dhh Mar 3, 2014

Choose a reason for hiding this comment

Uh oh!

dhh commented Mar 3, 2014

Uh oh!

lukaszx0 commented Mar 3, 2014

Uh oh!

dhh commented Mar 3, 2014

Uh oh!

dhh commented Mar 4, 2014

Uh oh!

lukaszx0 commented Mar 4, 2014

Uh oh!

pch commented Mar 4, 2014

Uh oh!

dhh commented Mar 4, 2014

Uh oh!

lukaszx0 commented Mar 4, 2014

Uh oh!

pch commented Mar 4, 2014

Uh oh!

dhh commented Mar 5, 2014

Uh oh!

dhh commented Mar 5, 2014

Uh oh!

dhh commented Mar 5, 2014

Uh oh!

lukaszx0 commented Mar 5, 2014

Uh oh!

dhh commented Mar 5, 2014

Uh oh!

pch commented Mar 8, 2014

Uh oh!

pch commented Mar 8, 2014

Uh oh!

dhh commented Mar 8, 2014

Uh oh!

pch commented Mar 8, 2014

Uh oh!

dhh commented Mar 8, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development