turbolinks / turbolinks-classic

Classic version of Turbolinks. Now deprecated in favor of Turbolinks 5.
MIT License
3.54k stars 428 forks source link

De-dupe array to avoid DOM corruption on partial page replacement #596

Closed arigesher closed 9 years ago

arigesher commented 9 years ago

A quick filter on the list of nodes to change. Without this, I'm seeing errors in swapNodes on this line (turbolinks.js.coffee:201):

existingNode.parentNode.replaceChild(targetNode, existingNode)

Basically, if you specify the id in options.change AND add the data-turbolinks-temporary attribute to the nodes you want replaced, they get added to the list of nodesToChange twice.

The first time through, everything works fine.

The second time through, the parents have already been removed from the nodes and an exception is thrown when trying to traverse existingNode.parentNode.

Here's how I am invoking Turbolinks.visit() to get this error:

$(document).on "ready page:load", ->
  calls = 0
  $('.list-group-item.list-item').click (event)->
    event.stopPropagation()
    event.preventDefault()
    Turbolinks.visit $(this).attr('href'),
      change: ['github_profile_detail']

    return false

Here's the markup:

<div id='github_profiles_list' class="global-grid-panel sticky-left" data-turbolinks-permanent>
  <%= render "profiles_list" %>
</div>
<div id='github_profile_detail' class="global-grid-content panel-on-left" data-turbolinks-temporary>
  <div class="row">
    <div class="col-lg-12">
      <%= render 'layouts/messages' %>
    </div>
  </div>
  <div class="row">
    <div class="col-lg-12">
      <% if @github_profile.nil? %>
        <p>
          You have not linked a GitHub Profile with your account.
        </p>
        <%= link_to("https://github.com/login/oauth/authorize?client_id=#{GITHUB_CONFIG["oauth_client_id"]}&state=#{session[:oauth_state]}") do %>
          <%= button_tag "Register", class: "btn btn-success" %>
        <% end %>
      <% else %>
        <%= render "profile" %>
      <% end %>
    </div>
  </div>
</div>

As a workaround, I just changed options.change to true, and that avoided the duplication.

This patch has a fix to avoid it from happening all together.

Thibaut commented 9 years ago

Thanks for the bug report. Could you add a test?

Thibaut commented 9 years ago

Fixed by 66b913ee4a5171df3c3755218f0d8b2c3b974835.