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 safari no styles #132

Merged
merged 4 commits into from
Aug 29, 2016
Merged

Fix safari no styles #132

merged 4 commits into from
Aug 29, 2016

Conversation

marutypes
Copy link
Contributor

@marutypes marutypes commented Aug 26, 2016

This PR fixes the sometimes the app breaks case where a user navigates several times within a TG application using 0.3.0.

The breakages were the result of Turbohead#update missing the upstream document's css when processing it. This only occurred randomly (somewhere between 1/10 and 1/20 times) and only on safari. It seems to be the result of:

extractTrackedAssets = (doc) ->
  for node in doc.head.children when node.dataset.turbolinksTrack?
    node

Where the result of extractTrackedAssets would be different than the new implementation:

extractTrackedAssets = (doc) ->
  return [].slice.call(doc.querySelectorAll('[data-turbolinks-track]'))

However, only sometimes, and only on safari. Debugging through or adding console.log to the loop steps would make the function perform as expected.

Since querySelector does not give you a live collection, it seems to be some strange browser level behaviour involving live collections of DOM elements. Similar flakiness was also addressed previously in #93.

In addition, there was some concern that we were not testing large numbers of subsequent navigations (though this is not specifically the issue as far as I can tell), so I've included additional tests for this case.

@marutypes marutypes added the bug label Aug 26, 2016
@@ -185,6 +185,33 @@ describe 'Turbolinks', ->
assertLinks(['foo.css'])
done()

it 'many subsequent navigations do not break head asset tracking', (done) ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we have a way to run Safari in CI, these don't seem valuable. They probably give a false sense of confidence to the next people who come along and try to update the head query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. I'll wipe them. :)

@GoodForOneFare
Copy link
Member

Changes LGTM. Should I 🎩?

@marutypes
Copy link
Contributor Author

Yep, absolutely 🎩 if you can.

If you point your repo at 0.3.0 on master and navigate around for a bit on safari you should lose all your styles. If you do the same with this branch you should be 🆗

@qq99
Copy link
Contributor

qq99 commented Aug 29, 2016

Changes LGTM 👍

@marutypes
Copy link
Contributor Author

Cool I think this is g2g then

@marutypes marutypes merged commit 34a2581 into master Aug 29, 2016
@marutypes marutypes deleted the fix-safari-no-styles branch August 29, 2016 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants