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

Let Session know it needs to reload #150

Closed
wants to merge 1 commit into from

Conversation

joemasilotti
Copy link
Member

When working with multiple Turbo instances across tabs data can easily become out of sync. Submitting a form on the first tab might change data currently displayed on the second. And signing in on one tab might change everything about the others!

Session.clearSnapshotCache() only works when navigating back to a cached page. It doesn't help if the active screen is being shown on a tab.

This PR introduced a new public function, Session.setNeedsReload(). When the session's underlying Visitable appears it reloads if this function was called earlier.


The name of the function feels a bit off. clearSnapshotCache() is perfect, it is action-oriented and easy to understand. Is there a similar name for setNeedsReload()?

Alternatively, we ignore Turbo.js's snapshot cache cleaning and always reload the page (when it appears). I don't think that would change much, if any, existing behavior.

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.
@pfeiffer
Copy link
Contributor

pfeiffer commented Feb 23, 2024

The name of the function feels a bit off. clearSnapshotCache() is perfect, it is action-oriented and easy to understand. Is there a similar name for setNeedsReload()?

What about markSnapshotAsStale()?

Just chiming in that a potentially less invasive action than reload would be to refresh the session instead, which would keep scroll and state if morphing is enabled. Would love to see a Session#refresh method being exposed!

@joemasilotti
Copy link
Member Author

@pfeiffer does #174 accomplish what you are talking about?

@pfeiffer
Copy link
Contributor

Not really, I was just suggesting to add a method to Session, which would refresh it (using Turbo 8 refreshes) instead of doing a reload, which is a ColdBoot.

This way stale content is updated while preserving scroll and state, if morphing is enabled.

@joemasilotti
Copy link
Member Author

Oh! I didn't catch that distinction. Great point on reload() vs. one triggered via Turbo 8.

This was referenced Feb 26, 2024
@joemasilotti
Copy link
Member Author

Closed in favor of #184 and #183.

@joemasilotti joemasilotti deleted the needs-reload branch February 28, 2024 19:22
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.

2 participants