Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@ The reported message contains the exact replacement tag, built from the loop's r
<% end %>
```

```
Prefer `<%= render partial: "user", collection: @users %>` over rendering a partial once per iteration. Collection rendering builds the partial once instead of for every item.
Collection rendering names the local after the partial, so when the loop passes the element under a different name the replacement carries an `as:` to keep the partial working:

```erb
<% @gems.each do |topic_gem| %>
<%= render partial: "gem_card", locals: { topic_gem: topic_gem } %>
<% end %>
```

Rendering an object directly reports the shorthand collection form instead:
Expand All @@ -26,15 +30,11 @@ Rendering an object directly reports the shorthand collection form instead:
<% end %>
```

```
Prefer `<%= render @users %>` over rendering a partial once per iteration. Collection rendering builds the partial once instead of for every item.
```

## Rationale

When a partial is rendered inside a loop, Action View looks the template up and sets up a fresh local scope on every iteration. Collection rendering does that work once and then reuses it for every element, so it is meaningfully faster for anything but the shortest collections.

Collection rendering also passes each element as a local named after the partial, and provides a `<partial>_counter` local, which removes the need to thread the loop variable through by hand.
Collection rendering also passes each element as a local named after the partial, and provides a `<partial>_counter` local, which removes the need to thread the loop variable through by hand. When the loop passes the element under a name that isn't the partial name, the suggestion adds `as:` so the partial keeps receiving the local it expects.

Because the rewrite emits the partial and nothing else, this rule only fires when the loop body is exactly one output `render` and the only local passed is the block argument. Loops that wrap the partial in markup, pass extra locals, or use a block argument the partial doesn't receive are left alone, since collection rendering cannot express them.

Expand All @@ -46,6 +46,10 @@ Because the rewrite emits the partial and nothing else, this rule only fires whe
<%= render partial: "user", collection: @users %>
```

```erb
<%= render partial: "gem_card", collection: @gems, as: :topic_gem %>
```

```erb
<%= render @users %>
```
Expand Down Expand Up @@ -90,6 +94,12 @@ Loops that do more than render a single partial are not flagged, because collect
<% end %>
```

```erb
<% @gems.each do |topic_gem| %>
<%= render partial: "gem_card", locals: { topic_gem: topic_gem } %>
<% end %>
```

## References

- [Action View Partials: Rendering Collections](https://guides.rubyonrails.org/layouts_and_rendering.html#rendering-collections)
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,21 @@ class PreferCollectionRenderVisitor extends BaseRuleVisitor {
const locals = keywords.locals ?? []
if (locals.length !== 1) return null

const [local] = locals as Array<{ value?: { content?: string } }>
const [local] = locals as Array<{ name?: { value?: string }, value?: { content?: string } }>
if (local.value?.content !== blockArgument) return null

return `<%= render partial: "${partial}", collection: ${receiver} %>`
const localName = local.name?.value
if (!localName) return null

const as = localName === this.collectionLocalNameFor(partial) ? "" : `, as: :${localName}`

return `<%= render partial: "${partial}", collection: ${receiver}${as} %>`
}

private collectionLocalNameFor(partial: string): string {
const name = partial.split("/").pop() ?? partial

return name.startsWith("_") ? name.slice(1) : name
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,54 @@ describe("actionview-prefer-collection-render", () => {
assertOffenses(html)
})

it("suggests `as:` when the local is not named after the partial", () => {
const html = dedent`
<% @gems.each do |topic_gem| %>
<%= render partial: "gem_card", locals: { topic_gem: topic_gem } %>
<% end %>
`

expectError('Prefer `<%= render partial: "gem_card", collection: @gems, as: :topic_gem %>` over rendering a partial once per iteration. Collection rendering builds the partial once instead of for every item.')

assertOffenses(html)
})

it("suggests `as:` when the local is not named after the partial in the shorthand form", () => {
const html = dedent`
<% @gems.each do |topic_gem| %>
<%= render "gem_card", topic_gem: topic_gem %>
<% end %>
`

expectError('Prefer `<%= render partial: "gem_card", collection: @gems, as: :topic_gem %>` over rendering a partial once per iteration. Collection rendering builds the partial once instead of for every item.')

assertOffenses(html)
})

it("does not suggest `as:` when the local matches the partial name in a nested path", () => {
const html = dedent`
<% @gems.each do |gem_card| %>
<%= render partial: "gems/gem_card", locals: { gem_card: gem_card } %>
<% end %>
`

expectError('Prefer `<%= render partial: "gems/gem_card", collection: @gems %>` over rendering a partial once per iteration. Collection rendering builds the partial once instead of for every item.')

assertOffenses(html)
})

it("suggests `as:` when the local does not match the partial name in a nested path", () => {
const html = dedent`
<% @gems.each do |topic_gem| %>
<%= render partial: "gems/gem_card", locals: { topic_gem: topic_gem } %>
<% end %>
`

expectError('Prefer `<%= render partial: "gems/gem_card", collection: @gems, as: :topic_gem %>` over rendering a partial once per iteration. Collection rendering builds the partial once instead of for every item.')

assertOffenses(html)
})

it("flags the object shorthand and suggests the shorthand collection form", () => {
const html = dedent`
<% @users.each do |user| %>
Expand Down
Loading