search

Shopify / maintenance_tasks

A Rails engine for queueing and managing data migrations.
MIT License
1.05k stars 76 forks source link

Fix statically resolving renders in Task#show #1053

Closed skipkayhil closed 4 months ago

skipkayhil commented 4 months ago

Rails tries to parse render calls in partials to generate a tree of dependencies that should be included in the cache key for a template/partial. The RenderParser is also used by gems such as ActionView::Precompiler and LooseErbs for similar purposes (identifying template/partial dependencies).

Currently, render @task.active_runs and render @runs_page.records cannot be resolved by the RenderParser (this is a documented shortcoming of the RenderParser).

This commit makes the two partial dependencies explicit as recommended in the ActionView::CacheHelper docs. This should not change the behavior of the renders, but it does enable the RenderParser to identify the partials being rendered.

etiennebarrie commented 4 months ago

Interesting, but we don't use caching in the engine views. Is it something that can be enabled from the application?

hmcguire-shopify commented 4 months ago

Interesting, but we don't use caching in the engine views. Is it something that can be enabled from the application?

I think you're right, I'm not sure of a way that caching could be used here. I'm mostly interested in the static analysis:

The RenderParser is also used by gems such as ActionView::Precompiler and LooseErbs for similar purposes (identifying template/partial dependencies).

etiennebarrie commented 4 months ago

Oh cool. This line will also need a similar template dependency declaration: https://github.com/Shopify/maintenance_tasks/blob/e56130a0425f78af0f80e30a67a0c6025ad3605e/app/views/maintenance_tasks/runs/_run.html.erb#L11

Makes me think we should look into strict locals as well.

hmcguire-shopify commented 4 months ago

https://github.com/rails/rails/pull/50944

I actually have a fix in the works for that one as well 😄

(We could add the Template Dependency: maintenance_tasks/runs/info/* comment in the meantime for older Rails versions though)