Add support for replacing tags in <head> #165

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@macournoyer

Add ability to mark head tags with data-turbolinks-replace.

This will remove the tag on page:change and replace it with the new version from the freshly fetched page.

Example use cases:

  • When session storing csrf-token might expire between page change, making the meta tag invalid.
  • Could be used in lieu of data-turbolinks-track to replace CSS instead of doing a full page reload.
Add ability to mark <head> tags with data-turbolinks-replace.
This will remove the tag on page change and replace it with the new version from the freshly fetched page.

This is useful for example with a csrf-token stored in a session that might expire. Or could be used instead of data-turbolinks-track to replace CSS instead of doing a full page reload.
@reed

This comment has been minimized.

Show comment
Hide comment
@reed

reed Jan 14, 2013

Collaborator

This is an interesting idea, but there are couple bugs in your implementation.

  1. Your method of appending the nodes to the head will not trigger script nodes to execute. You'll have to use the same method we use in executeScriptTags().
  2. You're not replacing the head tags when we fetch a page from the cache. In order to do that, you'll have to add the nodes with data-turbolinks-replace to the cache objects. Also, we don't want the script tags to execute when fetching from the cache, so we'll have to account for that as well.
  3. I'm sorry to nitpick, but you misspelled replaceable in extractReplaceableNodes.

I think you can fix these bugs by doing this:

fetchReplacement = (url) ->
  # ...
  else
    changePage extractTitleAndBody(doc)...
    replaceHeadNodes extractReplaceableNodes(doc.head), true
    reflectRedirectedUrl xhr
  # ...

fetchHistory = (state) ->
  # ...
  changePage page.title, page.body
  replaceHeadNodes page.replaceableHeadNodes
  recallScrollPosition page
  # ...

cacheCurrentPage = ->
  # ...
  pageCache[currentState.position] =
    url: document.location.href,
    # ...
    replaceableHeadNodes: extractReplaceableNodes document.head
  # ...

replaceHeadNodes = (nodes, execute) ->
  document.head.removeChild node for node in extractReplaceableNodes(document.head)
  for node in nodes
    if execute
      copy = document.createElement node.nodeName
      copy.setAttribute attr.name, attr.value for attr in node.attributes
      copy.appendChild document.createTextNode node.innerHTML
      document.head.appendChild copy
    else
      document.head.appendChild node
Collaborator

reed commented Jan 14, 2013

This is an interesting idea, but there are couple bugs in your implementation.

  1. Your method of appending the nodes to the head will not trigger script nodes to execute. You'll have to use the same method we use in executeScriptTags().
  2. You're not replacing the head tags when we fetch a page from the cache. In order to do that, you'll have to add the nodes with data-turbolinks-replace to the cache objects. Also, we don't want the script tags to execute when fetching from the cache, so we'll have to account for that as well.
  3. I'm sorry to nitpick, but you misspelled replaceable in extractReplaceableNodes.

I think you can fix these bugs by doing this:

fetchReplacement = (url) ->
  # ...
  else
    changePage extractTitleAndBody(doc)...
    replaceHeadNodes extractReplaceableNodes(doc.head), true
    reflectRedirectedUrl xhr
  # ...

fetchHistory = (state) ->
  # ...
  changePage page.title, page.body
  replaceHeadNodes page.replaceableHeadNodes
  recallScrollPosition page
  # ...

cacheCurrentPage = ->
  # ...
  pageCache[currentState.position] =
    url: document.location.href,
    # ...
    replaceableHeadNodes: extractReplaceableNodes document.head
  # ...

replaceHeadNodes = (nodes, execute) ->
  document.head.removeChild node for node in extractReplaceableNodes(document.head)
  for node in nodes
    if execute
      copy = document.createElement node.nodeName
      copy.setAttribute attr.name, attr.value for attr in node.attributes
      copy.appendChild document.createTextNode node.innerHTML
      document.head.appendChild copy
    else
      document.head.appendChild node
@reed

This comment has been minimized.

Show comment
Hide comment
@reed

reed Feb 25, 2013

Collaborator

@dhh what do you think of this? If he makes the changes I suggested, I like it. Gives developers more control in a manner that promotes the use of turbolinks, as opposed to providing ways to circumvent it.

Collaborator

reed commented Feb 25, 2013

@dhh what do you think of this? If he makes the changes I suggested, I like it. Gives developers more control in a manner that promotes the use of turbolinks, as opposed to providing ways to circumvent it.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 27, 2013

Contributor

I haven't heard a compelling use case yet. We have a way to deal with changing CSS and the crsf token shouldn't just be changing like that. What's the actual case driving this?

Contributor

dhh commented Feb 27, 2013

I haven't heard a compelling use case yet. We have a way to deal with changing CSS and the crsf token shouldn't just be changing like that. What's the actual case driving this?

@reed

This comment has been minimized.

Show comment
Hide comment
@reed

reed Feb 28, 2013

Collaborator

I think the submitter should defend his idea, but I'll throw this out there:

There are plenty of third-party API's that use page-specific meta tags, like Facebook's Open Graph or Twitter Cards (both of which you'd find in the source of this page). I think this would make compatibility easier to achieve.

Our current method of handling asset changes just falls back to a normal page load if there are any changes. Using this solution would allow assets to change without abandoning the turbolinks session.

Collaborator

reed commented Feb 28, 2013

I think the submitter should defend his idea, but I'll throw this out there:

There are plenty of third-party API's that use page-specific meta tags, like Facebook's Open Graph or Twitter Cards (both of which you'd find in the source of this page). I think this would make compatibility easier to achieve.

Our current method of handling asset changes just falls back to a normal page load if there are any changes. Using this solution would allow assets to change without abandoning the turbolinks session.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 28, 2013

Contributor

You can still have per-page meta tags, though, that work with first-hit requests. Doesn't that satisfy Twitter cards etc?

Thing is, if you constantly need to reload, then what is turbo links doing for you?

On Feb 27, 2013, at 6:24 PM, Nick Reed notifications@github.com wrote:

I think the submitter should defend his idea, but I'll throw this out there:

There are plenty of third-party API's that use page-specific meta tags, like Facebook's Open Graph or Twitter Cards (both of which you'd find in the source of this page). I think this would make compatibility easier to achieve.

Our current method of handling asset changes just falls back to a normal page load if there are any changes. Using this solution would allow assets to change without abandoning the turbolinks session.


Reply to this email directly or view it on GitHub.

Contributor

dhh commented Feb 28, 2013

You can still have per-page meta tags, though, that work with first-hit requests. Doesn't that satisfy Twitter cards etc?

Thing is, if you constantly need to reload, then what is turbo links doing for you?

On Feb 27, 2013, at 6:24 PM, Nick Reed notifications@github.com wrote:

I think the submitter should defend his idea, but I'll throw this out there:

There are plenty of third-party API's that use page-specific meta tags, like Facebook's Open Graph or Twitter Cards (both of which you'd find in the source of this page). I think this would make compatibility easier to achieve.

Our current method of handling asset changes just falls back to a normal page load if there are any changes. Using this solution would allow assets to change without abandoning the turbolinks session.


Reply to this email directly or view it on GitHub.

@macournoyer

This comment has been minimized.

Show comment
Hide comment
@macournoyer

macournoyer Feb 28, 2013

My use case for this was when the csrf token changes because the session has been reseted on the server.

Eg.: using a memcache session store w/ 2 hours expiry

  1. User browse to page.
  2. Leaves page opened for > 2h.
  3. Goes back to page. Browse around.
  4. Submits an ajax form or anything that uses the csrf meta tags.

Now csrf token in meta tag is invalid and will stay invalid until a full page refresh is triggered by the user or some other code.

However this is no longer an issue for me, we've since gone back to the cookie store. And of course could be solved by increasing expiry too.

My use case for this was when the csrf token changes because the session has been reseted on the server.

Eg.: using a memcache session store w/ 2 hours expiry

  1. User browse to page.
  2. Leaves page opened for > 2h.
  3. Goes back to page. Browse around.
  4. Submits an ajax form or anything that uses the csrf meta tags.

Now csrf token in meta tag is invalid and will stay invalid until a full page refresh is triggered by the user or some other code.

However this is no longer an issue for me, we've since gone back to the cookie store. And of course could be solved by increasing expiry too.

@reed

This comment has been minimized.

Show comment
Hide comment
@reed

reed Feb 28, 2013

Collaborator

Yeah, you're right. I was under the (false) impression that those meta tags (for FB and Twitter) are used by the javascript they have you embed in your page. I still wouldn't be surprised if there are libraries that do use meta tags like that, but without an example, my argument is dead on that front.

I think this solution has value, but I don't feel strongly enough about it to continue advocating for it if Marc-Andre doesn't need it anymore.

Collaborator

reed commented Feb 28, 2013

Yeah, you're right. I was under the (false) impression that those meta tags (for FB and Twitter) are used by the javascript they have you embed in your page. I still wouldn't be surprised if there are libraries that do use meta tags like that, but without an example, my argument is dead on that front.

I think this solution has value, but I don't feel strongly enough about it to continue advocating for it if Marc-Andre doesn't need it anymore.

@cmer

This comment has been minimized.

Show comment
Hide comment

cmer commented Mar 5, 2013

👍

@reed

This comment has been minimized.

Show comment
Hide comment
@reed

reed Apr 3, 2013

Collaborator

Closing in favor of #200

Collaborator

reed commented Apr 3, 2013

Closing in favor of #200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment