Skip to content

Conversation

@alexspeller
Copy link

When users rapidly navigate between pages with deferred props, multiple requests fire in quick succession. Previously, deferred prop requests from previous navigations were not cancelled, causing them to complete out-of-order and display stale data.

This change cancels all in-flight async (deferred) requests when starting a new main visit, while preserving concurrent loading of deferred props within the same page load.

Changes:

  • Fix RequestStream.cancelInFlight() to cancel ALL requests (not just first)
  • Add cancellation logic in Router.visit() after onBefore check
  • Add detection to distinguish deferred requests from main navigation
  • Add tests suite for cancellation behavior

The cancellation happens after onBefore checks to avoid cancelling deferred props when navigation is prevented by user confirmation dialogs.

@alexspeller alexspeller force-pushed the fix/cancel-deferred-props-on-navigation branch from aecd016 to f9fcbfa Compare October 24, 2025 11:48
When users rapidly navigate between pages with deferred props, multiple
requests fire in quick succession. Previously, deferred prop requests
from previous navigations were not cancelled, causing them to complete
out-of-order and display stale data.

This change cancels all in-flight async (deferred) requests when
starting a new main visit, while preserving concurrent loading of
deferred props within the same page load.

Changes:
- Fix RequestStream.cancelInFlight() to cancel ALL requests (not just first)
- Add cancellation logic in Router.visit() after onBefore check
- Add detection to distinguish deferred requests from main navigation
- Add comprehensive test suite for cancellation behavior

The cancellation happens after onBefore checks to avoid cancelling
deferred props when navigation is prevented by user confirmation dialogs.
@alexspeller alexspeller force-pushed the fix/cancel-deferred-props-on-navigation branch from f9fcbfa to f002a1e Compare October 24, 2025 11:54
@pascalbaljet
Copy link
Member

Thanks for this! I'm going to investigate it, though I must admit I'm a little scared about the 1000-line test file 😅

I haven't checked in detail yet, but is this related to #2108 as well?

@alexspeller
Copy link
Author

Yeah routing stuff always ends up with loads of edge cases (I did a bunch of work on the Ember.js router back in the day) so lots of tests ftw - a lot of them ended up being about the interaction with prefetch and deferred - there's just so many possible combinations of when things should vs shouldn't be cancelled. Hopefully the test cases are easy enough to follow even though there's loads of them!

Arguably the amount of test cases could be reduced, although each of them documents the desired behaviour given a complex scenario, so whilst it's a lot to read, I think it probably makes sense to see if at least on the face of it the title of each test case fits with the inertia routing vision - I think so but it's a lot easier to see that written out in a test case than infer it from the relatively minimal code changes here.

They might also expose some edge-case inconsistencies that are unrelated - for example what happens if you pass both only and except to visit? - is that even a supported use case? Maybe it's not worth testing that as part of this PR, but I wanted to make sure that at least something sane would happen when doing so.

I don't think #2108 is related to this - unless this change fixes it by total coincidence. I would expect that it doesn't, as that issue is saying there should be more deferrred requests in some circumstances (when preloading) and theoretically this PR should only result in less (completed) deferred requests in the case that they are stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants