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

Stale content #184

Merged
merged 2 commits into from
Feb 28, 2024
Merged

Stale content #184

merged 2 commits into from
Feb 28, 2024

Conversation

joemasilotti
Copy link
Member

This PR exposes two ways of clearing stale content in turbo-ios:

  1. markSnapshotCacheAsStale()
  2. markContentAsStale()

Due to a race condition discovered in #173, clearing the snapshot cache must occur when the visitable view appears. This prevents developers from calling clearSnapshotCache() directly from sessionDidFinishFormSubmission(_:): #183.

It is helpful to let other Session instances know that data on the
server changed and what they are rendering might be out of date. This is
mostly useful when there are tabs.
@joemasilotti
Copy link
Member Author

@jayohms, is there an Android equivalent needed for this addition? From what I can see there doesn't seem to be an equivalent clearSnapshotCache() on Android.

@jayohms
Copy link
Collaborator

jayohms commented Feb 28, 2024

@joemasilotti since default and modal visits occur in the same session, I don't see a need on the Android side. So no worries there 👍

@joemasilotti
Copy link
Member Author

Excellent, thanks @jayohms!

@joemasilotti joemasilotti merged commit c96149f into main Feb 28, 2024
1 check passed
@joemasilotti joemasilotti deleted the stale-content branch February 28, 2024 21:37
@pfeiffer
Copy link
Contributor

pfeiffer commented Feb 29, 2024

@jayohms @joemasilotti I believe that corresponding methods would be appropriate on the Android side as well, the use-case being a native screen mutating some data in which case the session(s) would have stale data.

Without these methods on the Android side, native screens would not be able to mark snapshots as stale, right?

Comment on lines 242 to +250
} else if visitable !== topmostVisit.visitable {
// Navigating backward
visit(visitable, action: .restore)
} else if isShowingStaleContent {
reload()
isShowingStaleContent = false
} else if isSnapshotCacheStale {
clearSnapshotCache()
isSnapshotCacheStale = false
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work on the Hiking Journal demo.
The condition on L242 is different than in #173 (comment).
Why are not the flags evaluated separately after the guard statement?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I just confirmed this PR doesn't actually fix the core issue.

I just opened #185 - thanks!

olivaresf added a commit that referenced this pull request Mar 1, 2024
…s-response

* origin/main:
  Fix when snapshot cache is cleared (#185)
  Stale content (#184)
  Note feature parity in CONTRIBUTING doc
  Provide backwards compatibility support for Turbo 7.x and Turbolinks 5
  Support visit.isPageRefresh and avoid displaying the activity indicator and screenshots when the page is refreshing
  Support Turbo 8 same-page refreshes and include debug logging
  rename loadrRequest to load (#141)
  Add CONTRIBUTING.md (#90)
  Update README window.Turbo reference (#68)

# Conflicts:
#	Source/Session/Session.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants