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

Add more prefetch conditions #1178

Merged
merged 5 commits into from Feb 9, 2024
Merged

Add more prefetch conditions #1178

merged 5 commits into from Feb 9, 2024

Conversation

afcapel
Copy link
Collaborator

@afcapel afcapel commented Feb 8, 2024

This PR adds some more conditions where a link is not considered safe to be prefetched.

The risk/reward ratio favors a more conservative approach. If we don't prefetch a link the interaction can be a bit slower, but if we prefetch a link that is not safe, we may trigger a bug.

The conditions are quite specific, in any case, so they shouldn't trigger often.

The new conditions are:

  • If the link is a UJS link (i.e. it has a data-remote attribute).
  • If the link has a data-turbo-confirm attribute.
  • If the link has a data-turbo-stream attribute.

The PR also cleans up the code a bit, extracting each of the conditions into helper methods.

Fixes #1170
Fixes #1174

Copy link
Collaborator

@kevinmcconnell kevinmcconnell left a comment

Choose a reason for hiding this comment

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

Looks great!

cancelable: true
})
if (unfetchableLink(link)) return false
if (linkToTheSamePage(link)) return false
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we discussed IRL, I'm not sure we should always exclude the current page. Sometimes clicking navigation to the same page is a valid way of getting latest content on a page, and in that case prefetching seems valid.

Perhaps there are edge cases to think about when fetching them, though. In any case, probably fine to leave for another PR, just wanted to note is since we're touching this part.

Copy link

Choose a reason for hiding this comment

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

You could probably argue that if the current page is prefetched it might as well morph it to the latest version. Though there are probably edge cases as you say.

Comment on lines +146 to +148
if (linkOptsOut(link)) return false
if (nonSafeLink(link)) return false
if (eventPrevented(link)) return false
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is much nicer with these checks extracted and given meaningful names, btw! 👏

@seanpdoyle
Copy link
Contributor

Should we also open PR to remove https://github.com/hotwired/turbo-site/blob/694d442e6e57208814803cb5b2536e93bb398cd6/_source/handbook/02_drive.md#L369-L374 from the examples for canceling a prefetch?

Copy link

@ur5us ur5us left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this so quickly.

cancelable: true
})
if (unfetchableLink(link)) return false
if (linkToTheSamePage(link)) return false
Copy link

Choose a reason for hiding this comment

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

You could probably argue that if the current page is prefetched it might as well morph it to the latest version. Though there are probably edge cases as you say.

@afcapel afcapel merged commit 38e7bd2 into main Feb 9, 2024
2 checks passed
@afcapel afcapel deleted the more-prefetch-conditions branch February 9, 2024 09:28
afcapel added a commit to hotwired/turbo-site that referenced this pull request Feb 9, 2024
That kind of check is not needed since hotwired/turbo#1178
afcapel added a commit to hotwired/turbo-site that referenced this pull request Feb 9, 2024
That kind of check is not needed since hotwired/turbo#1178
@afcapel
Copy link
Collaborator Author

afcapel commented Feb 9, 2024

@seanpdoyle good point, I've removed the example in hotwired/turbo-site#162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants