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 partial replace head tracking #133

Merged
merged 4 commits into from
Aug 29, 2016

Conversation

marutypes
Copy link
Contributor

@marutypes marutypes commented Aug 29, 2016

This PR fixes partial replaces inadvertently triggering head asset updates. This was usually not causing issues, but in some cases could break stuff.

I should note that this was partially to blame for the spike in url.absolute errors. Submitting a remote form would call:

        Page.refresh
          response: xhr
          onlyKeys: @refreshOnSuccess

Page.refresh would then trigger

    options.partialReplace = true
    options.onLoadFunction = callback

    xhr = options.response
    delete options.response
    Turbolinks.loadPage null, xhr, options

Which would then try to update the head assets, with an error callback that looked like:

onHeadUpdateError = ->
          Turbolinks.fullPageNavigate(url.absolute)

Where url is null.

Preventing head asset updates on partial request should fix the new instances of this error that all occurred with a tg-remote related last event.

The old instances of this error are a bit more insidious and will be addressed in an upcoming PR.

@marutypes marutypes added the bug label Aug 29, 2016
Turbolinks.fullPageNavigate(url.absolute)
)
return updateBody(upstreamDocument, xhr, options)
Turbolinks.fullPageNavigate(url.absolute)
Copy link
Contributor

@qq99 qq99 Aug 29, 2016

Choose a reason for hiding this comment

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

your logic changes LGTM
My only comment would be that I prefer the style of explicit if/else rather than return throughout to terminate control flow

@GoodForOneFare
Copy link
Member

Makes sense 👍

@marutypes
Copy link
Contributor Author

I'll 🚢 this when it's done on CI

@marutypes marutypes force-pushed the fix-partial-replace-head-tracking branch from bdb1eb6 to 96c69ce Compare August 29, 2016 20:47
@marutypes
Copy link
Contributor Author

Had to rebase on top of the other two prs

@marutypes marutypes merged commit 5ac5d58 into master Aug 29, 2016
@marutypes marutypes deleted the fix-partial-replace-head-tracking branch August 29, 2016 20:53
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.

3 participants