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: don't wrongly abort preload fetch requests #124

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

hirasso
Copy link
Member

@hirasso hirasso commented Feb 2, 2024

The problem

Currently, all preload requests after the first page:view are being exited early. The reason for this is that swup.fetchPage falls back to swup's internal visit object if we don't provide a custom visit to it. It then calls the hook fetch:request that exits early if the visit is already done.

The solution

Pass the temporary visit from performPreload along in the FetchOptions for fetchPage.

Future

  • should we make the visit a required FetchOption so that we don't run into this again?
  • should we make FetchOptions.visit public?

I'd like to merge this as soon as possible, we can discuss possible improvements later.

Checks

  • The PR is submitted to the master branch
  • The code was linted before pushing (npm run lint)
  • All tests are passing (npm run test)

@hirasso hirasso changed the title fix: don't wrongly abort preload requests fix: don't wrongly abort preload fetch requests Feb 2, 2024
@hirasso hirasso requested a review from daun February 2, 2024 12:45
Copy link

github-actions bot commented Feb 2, 2024

Playwright test results

passed  88 passed
skipped  4 skipped

Details

stats  92 tests across 8 suites
duration  1 minute, 25 seconds
commit  a0422a6

Skipped tests

chromium → preload-plugin.spec.ts → active links → preloads links on touchstart
firefox → preload-plugin.spec.ts → active links → preloads links on touchstart
webkit → preload-plugin.spec.ts → active links → preloads links on touchstart
ios → preload-plugin.spec.ts → active links → preloads links on hover

@daun
Copy link
Member

daun commented Feb 2, 2024

Nice! I'd tend to answer no to both questions – the FetchOptions interface is also part of the public API of navigate() and I'd rather keep that clean. We might refactor the whole thing though because currently it's a bit messy 🤠

@hirasso hirasso merged commit ebdb2e3 into master Feb 2, 2024
2 checks passed
@hirasso hirasso deleted the fix/fetch-page-visit-done branch February 2, 2024 12:49
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