Possibly problematic defaults in menu templates

[oneiros]

Sorry for not using the template. I found the following hard to get into that format.

The default templates for menu views include caching. This is the current (slim) version for a single node:

alchemy_cms/lib/generators/alchemy/menus/templates/node.html.slim

Lines 1 to 13 in 137701d

	- cache node do
	= content_tag :li,
	class: ['nav-item', node.children.any? ? 'dropdown' : nil].compact do
	= link_to_if node.url,
	node.name,
	@preview_mode ? 'javascript: void(0)' : node.url,
	class: ['nav-link', current_page?(node.url) ? 'active' : nil].compact,
	title: node.title,
	target: node.external? ? '_blank' : nil,
	rel: node.nofollow? ? 'nofollow' : nil
	- if node.children.any?
	ul.dropdown-menu
	= render node.children.includes(:page, :children), as: 'node'

It uses the node itself as cache key, which seems fine at first, but upon closer inspection, there are two more things affecting the actual display of the node: An active class is added if the node is the current page and the href is replaced with javascript: void(0) in preview mode.

We stumbled upon this when in production the navigation would sometimes stop working. It was always the case when an editor opened a page in preview mode before any normal users accessed a page with the same menu. In that case the javascript: void(0) landed in the cached version ☹️

We solved this by composing a more complex cache key from [node, @page, @preview_mode]. I am not sure this is the most elegant solution, especially as this needs to be repeated in the wrapper template.

All in all this is no big deal as most sites will probably make heavy modifications to these templates anyway. Just wanted to let you know that these defaults might not be ideal.

[tvdeyen]

Yes, this is an issue.

You are able to build your own cache key like you already described, but we should update the default templates. Would you mind to open a PR?

Thanks for bringing this into attention!