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

Fix digesting templates with mixed formats #32282

Merged
merged 1 commit into from Mar 20, 2018

Conversation

@javan
Copy link
Member

@javan javan commented Mar 18, 2018

Fixes #28503
Follow up to: #25411

@javan javan added the actionview label Mar 18, 2018
@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 18, 2018

I've tested this against the repro I created when I reported #28503 and this PR resolves the issue. I've been unable to immediately test this against the production app where I first noticed this issue as the app doesn't play nice with rails master presently. Hopefully this can be backported against 5.1 and 5.2 - which I'm happy to help with.

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 18, 2018

Having now backported the fix to 5.1 stable I can confirm that this fixes the issue I was experiencing.

@kaspth
kaspth approved these changes Mar 18, 2018
Copy link
Member

@kaspth kaspth left a comment

Nice! Feel free to backport to 5-2-stable and 5-1-stable.

@javan
Copy link
Member Author

@javan javan commented Mar 18, 2018

I think I've found a better solution that aligns more closely with the behavior of rendering: 052a6d5

By setting finder.formats, we avoid the need to find with a :formats option. And when it's set to [:js], we automatically get an :html fallback:

# add :html as fallback to :js.
def formats=(values)
if values
values.concat(default_formats) if values.delete "*/*".freeze
if values == [:js]
values << :html
@html_fallback_for_js = true
end
end
super(values)
end

Rendering a request works similarly:

# Assign the rendered format to look up context.
def _process_format(format)
super
lookup_context.formats = [format.to_sym]
lookup_context.rendered_format = lookup_context.formats.first
end

@thomasfedb, mind trying once more with my latest commit? If it doesn't pan out I'll use my first approach.

@kaspth, if you have any other ideas, please do let me know! It's a jungle in there.

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 18, 2018

@javan would that break if I, for example, tried to render a .txt partial inside a .js partial? Or some other cross-format combination?

@javan
Copy link
Member Author

@javan javan commented Mar 19, 2018

I think that will work, but please do investigate.

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 19, 2018

@javan I'll give it a try

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 19, 2018

@javan new fix definitely solves the issue when using HTML partials inside JS views as with the previous fix.

There is an issue, however, when you render a partial with a particular format, e.g. partial: 'partial2', formats: :text, collection: [1], cached: true which results in a Couldn't find template for digesting: home/_partial2 message, and a blank cache key is used (and therefore old cache is still used when partial is updated).

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 19, 2018

The above happens inside both JS and HTML views, as you'd expect.

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 19, 2018

Sorry, should make it clear that this issue with rendering with specified format appears only to occur with the new fix, and was not a problem with the initial fix.

With initial fix:

Processing by HomeController#index as HTML
  Rendering home/index.html.erb within layouts/application
Read fragment views/home/_partial2:140b891b4a188473f1a3d632568d6824/localhost:3000/ (0.0ms)
Write fragment views/home/_partial2:140b891b4a188473f1a3d632568d6824/localhost:3000/ (0.0ms)
  Rendered collection of home/_partial2.text.erb [0 / 1 cache hits] (1.8ms)
  Rendered home/index.html.erb within layouts/application (14.6ms)
Completed 200 OK in 1053ms (Views: 1051.9ms | ActiveRecord: 0.0ms)

With new fix:

Processing by HomeController#index as HTML
  Rendering home/index.html.erb within layouts/application
  Couldn't find template for digesting: home/_partial2
Read fragment views/home/_partial2/localhost:3000/ (0.1ms)
Write fragment views/home/_partial2/localhost:3000/ (0.1ms)
  Rendered collection of home/_partial2.text.erb [0 / 1 cache hits] (1.6ms)
  Rendered home/index.html.erb within layouts/application (14.4ms)
Completed 200 OK in 1037ms (Views: 1036.6ms | ActiveRecord: 0.0ms)

Let me know if you need further info @javan

@javan
Copy link
Member Author

@javan javan commented Mar 19, 2018

Thanks @thomasfedb! It looks like DependencyTracker doesn't parse the :formats option currently. So my initial fix happens to work correctly, but would probably fail if you had more than one _partial2 template. If you had _partial2.text.erb and _partial2.html.erb it would always digest the html template.

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 19, 2018

@javan right, there was only _partial2.text.erb in my test, no other partial called partial2. I'd be happy to go with either of your fixes for now, what do you think?

@javan javan force-pushed the javan:fix-digesting-mixed-formats branch to f4eb2e2 Mar 20, 2018
@javan javan merged commit 7f71a6a into rails:master Mar 20, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@javan
Copy link
Member Author

@javan javan commented Mar 20, 2018

Went with the 2nd approach. Thanks for all your help @thomasfedb!

javan added a commit that referenced this pull request Mar 20, 2018
Fix digesting templates with mixed formats
javan added a commit that referenced this pull request Mar 20, 2018
Fix digesting templates with mixed formats
@javan
Copy link
Member Author

@javan javan commented Mar 20, 2018

Backported to 5-2 0b53f57 and 5-1 eb30975

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 20, 2018

Thanks @javan, and clue as to when the next 5.1.x will be cut?

@javan
Copy link
Member Author

@javan javan commented Mar 20, 2018

I'm not sure about the 5-1 release schedule. Maybe @rafaelfranca knows? You can always point your Gemfile at the Rails branch or sha.

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Mar 20, 2018

Using the branch now in dev, but would prefer to have an official release before I deploy to prod.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 20, 2018

It may takes months. We usually release every 6 weeks, but given 5.2 is going to be the official release, it may take more time.

@erikthiem
Copy link

@erikthiem erikthiem commented Dec 19, 2018

Thanks @thomasfedb! It looks like DependencyTracker doesn't parse the :formats option currently. So my initial fix happens to work correctly, but would probably fail if you had more than one _partial2 template. If you had _partial2.text.erb and _partial2.html.erb it would always digest the html template.

@javan This PR broke a project I'm working on because I had a similar case to your example ("more than one _partial2 template"). Do you know if there is a fix to get the code change from this PR working when you have multiple templates with the same name but different format endings?

@thomasfedb
Copy link
Contributor

@thomasfedb thomasfedb commented Dec 25, 2018

@erikthiem I think you'd need to make DependencyTracker aware of :formats to avoid that issue.

rdunlop added a commit to rdunlop/unicycling-registration that referenced this pull request Jan 12, 2019
I think that this may have been solved by.
rails/rails#28503

rails/rails#32282
rdunlop added a commit to rdunlop/unicycling-registration that referenced this pull request Jan 12, 2019
I think that this may have been solved by.
rails/rails#28503

rails/rails#32282
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.

None yet

5 participants