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 statically resolving renders in Task#show #1053

Merged

Conversation

skipkayhil
Copy link
Contributor

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.

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.
@hmcguire-shopify hmcguire-shopify merged commit a13741b into Shopify:main Jul 16, 2024
21 checks passed
@skipkayhil skipkayhil deleted the hm-fix-task-show-dep-tracking branch July 16, 2024 20:10
@etiennebarrie
Copy link
Member

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

@hmcguire-shopify
Copy link

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
Copy link
Member

Oh cool. This line will also need a similar template dependency declaration:

<%= render "maintenance_tasks/runs/info/#{run.status}", run: run %>

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

@hmcguire-shopify
Copy link

hmcguire-shopify commented Jul 24, 2024

rails/rails#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)

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

Successfully merging this pull request may close these issues.

4 participants