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

Location getter + abortable check #904

Closed
wants to merge 15 commits into from
Closed

Location getter + abortable check #904

wants to merge 15 commits into from

Conversation

daun
Copy link
Member

@daun daun commented Jun 9, 2024

Description

  • Re-implement some changes from the visit:abort branch that make sense

Location object

  • Store the current location as a Location object at swup.location
  • Allows checking the current hash in addition to just the path/query
  • Save the hash of the previous page in the visit object
  • Deprecate swup.currentPageUrl in favor of swup.location.url
if (swup.location.url === '/about') {}
if (swup.location.hash) {}
if (visit.from.hash === '#detail') {}

Location update

  • Update browser url at earliest possible time

Aborting visits

  • Add visit.abortable getter that defines when a visit can be aborted
  • Visits that have started entering cannot be aborted
  • History visits cannot be aborted (wrong assumption, needs to be abortable)
  • Refactor aborting visits from swup.navigate()

Note to self

  • Not allowing aborting history doesn't make too much sense here (we only abort visits if there's a new one)

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)
  • New or updated tests are included
  • The documentation was updated as required

Copy link

github-actions bot commented Jun 9, 2024

Size Change: +175 B (+0.76%)

Total Size: 23.1 kB

Filename Size Change
dist/Swup.modern.js 6.99 kB +54 B (+0.78%)
dist/Swup.module.js 7.91 kB +61 B (+0.78%)
dist/Swup.umd.js 8.17 kB +60 B (+0.74%)

compressed-size-action

Copy link

github-actions bot commented Jun 9, 2024

Playwright test results

passed  303 passed
skipped  3 skipped

Details

report  Open report ↗︎
stats  306 tests across 22 suites
duration  1 minute, 5 seconds
commit  4d4e3fb

Skipped tests

webkit → animation-timing.spec.ts → animation timing → detects animation timing
webkit → animation-timing.spec.ts → animation timing → detects complex animation timing
webkit → animation-timing.spec.ts → animation timing → detects keyframe timing

@hirasso
Copy link
Member

hirasso commented Jun 9, 2024

Not allowing aborting history doesn't make too much sense here (we only abort visits if there's a new one)

It should read "allowing", not "Not allowing", right? :)

Also: this only holds true for animateHistoryBrowsing === false. I actually wouldn't mind deprecating that option sometime, anyways...

@daun
Copy link
Member Author

daun commented Jun 9, 2024

@hirasso No, I think that's what I actually meant to write. In the following sense: by not allowing history visits to be aborted, we now make it impossible to supersede history visits. That would be the case when clicking the back button a few times in quick succession. If the cache is disabled or history visits are animated, every one of these history visits will have to finish completely before queueing the next one. Hence, we should allow aborting history visits if there's a new visit. I need to remove that part from this PR.

The really not allowing to abort them was only necessary in the previous PR where one could abort a visit from anywhere at any point. But even there we should have added a special condition for quick history visits in succession.

This is all very confusing :/ I really wish we had a nice state machine 🤖

@hirasso
Copy link
Member

hirasso commented Jun 9, 2024

It's indeed pretty confusing... thank you for clarifying what exactly you meant.

@daun daun marked this pull request as ready for review June 14, 2024 12:15
@daun daun requested a review from a team June 14, 2024 12:16
@daun
Copy link
Member Author

daun commented Jun 14, 2024

Addressed the open points and fixed the tests. Ready for review :)

@daun daun changed the title Location getter + Abortable visits Location getter + abortable check Jun 14, 2024
visit.state = VisitState.STARTED;

// Create/update history record if this is not a popstate call or leads to the same URL
Copy link
Member

Choose a reason for hiding this comment

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

Moving the bit where the URL updates feels pretty scary to me. I can recall various situations where we discovered quite late in the process that the exact moment where the URL was updated had a lot of implications.

Why did you move it?

@daun
Copy link
Member Author

daun commented Jun 19, 2024

This is doing a bit too much — slimmed down PR incoming :)

@daun daun closed this Jun 19, 2024
@daun daun mentioned this pull request Jun 19, 2024
5 tasks
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.

None yet

2 participants