Repository: swup/swup
Pull request #776 (2023-08-24 07:55:04)

Add unique id to visit object
**Description**

- Add a unique random `visit.id` when creating new visits
- Ignore outdated visits if IDs mismatch
- As much as possible, refer to local `visit` var instead of `this.visit` to avoid modifying newer visit
- Add tests for the shape of the Visit object
- Add tests for outdated visits being ignored

**Goals**

- Identify and ignore outdated visits with more certainty
- Allow checking if we're still in the same visit in `hooks.once()` calls

**Allow identifying outdated visits**

Currently, we're checking if a newer visit was started in the meantime by comparing the two requested URLs. That doesn't work for the now-possible same-link visits, and it's also safer and clearer to just compare IDs:

```diff
- if (!this.isSameResolvedUrl(getCurrentUrl(), requestedUrl)) return;
+ if (visit.id !== this.visit.id) return;
```

**Allow checking for same visit in `once` hooks**

Currently, when registering a hook `once`, there is no guarantee that the run hook belongs to the same visit, e.g.:

- When clicking links twice in quick succession, it's possible for the second link to fetch earlier
- When hooking into animation functions that might be skipped, we'd get the animation of the **next** visit

By having a unique id on the visit object we can check in `once` hooks if we're indeed dealing with the same visit:

```js
swup.hooks.on('visit:start', (visit) => {
  if (someCondition) {
    swup.hooks.once('animation:in:await', ({ id }) => {
      if (visit.id !== id) return;
      await someCustomFunction();
    }
  }
});
```

**Checks**

- [x] The PR is submitted to the `master` branch
- [x] The code was linted before pushing (`npm run lint`)
- [x] All tests are passing (`npm run test`)
- [x] New or updated tests are included
- [ ] ~~The documentation was updated as required~~



Patch:
diff --git a/cypress/support/commands.js b/cypress/support/commands.js
index 33a2e37..70a1991 100644
--- a/cypress/support/commands.js
+++ b/cypress/support/commands.js
@@ -162,3 +162,10 @@ Cypress.Commands.add('pushHistoryState', (url, data = {}) => {
 		window.history.pushState(state, '', url);
 	});
 });
+
+Cypress.Commands.add('delayRequest', (url, delay) => {
+	cy.intercept({ pathname: url, times: 1 }, (req) => {
+		const { pathname: fixture } = new URL(req.url);
+		req.reply({ fixture, delay });
+	});
+});
diff --git a/src/modules/Visit.ts b/src/modules/Visit.ts
index 61313f9..5ddb485 100644
--- a/src/modules/Visit.ts
+++ b/src/modules/Visit.ts
@@ -3,6 +3,8 @@ import { HistoryAction, HistoryDirection } from './navigate.js';
 
 /** An object holding details about the current visit. */
 export interface Visit {
+	/** A unique ID to identify this visit */
+	id: number;
 	/** The previous page, about to leave */
 	from: VisitFrom;
 	/** The next page, about to enter */
@@ -92,6 +94,7 @@ export function createVisit(
 	{ to, from = this.currentPageUrl, hash, el, event }: VisitInitOptions
 ): Visit {
 	return {
+		id: Math.random(),
 		from: { url: from },
 		to: { url: to, hash },
 		containers: this.options.containers,
diff --git a/src/modules/navigate.ts b/src/modules/navigate.ts
index 1841808..63d4d99 100755
--- a/src/modules/navigate.ts
+++ b/src/modules/navigate.ts
@@ -61,37 +61,41 @@ export function navigate(
 export async function performNavigation(
 	this: Swup,
 	options: NavigationOptions & FetchOptions = {}
-) {
-	const { el } = this.visit.trigger;
+): Promise<void> {
+	// Save this localy to a) allow ignoring the visit if a new one was started in the meantime
+	// and b) avoid unintended modifications to any newer visits
+	const visit = this.visit;
+
+	const { el } = visit.trigger;
 	options.referrer = options.referrer || this.currentPageUrl;
 
 	if (options.animate === false) {
-		this.visit.animation.animate = false;
+		visit.animation.animate = false;
 	}
 
 	// Clean up old animation classes
-	if (!this.visit.animation.animate) {
+	if (!visit.animation.animate) {
 		this.classes.clear();
 	}
 
 	// Get history action from option or attribute on trigger element
 	const history = options.history || el?.getAttribute('data-swup-history') || undefined;
 	if (history && ['push', 'replace'].includes(history)) {
-		this.visit.history.action = history as HistoryAction;
+		visit.history.action = history as HistoryAction;
 	}
 
 	// Get custom animation name from option or attribute on trigger element
 	const animation = options.animation || el?.getAttribute('data-swup-animation') || undefined;
 	if (animation) {
-		this.visit.animation.name = animation;
+		visit.animation.name = animation;
 	}
 
 	// Sanitize cache option
 	if (typeof options.cache === 'object') {
-		this.visit.cache.read = options.cache.read ?? this.visit.cache.read;
-		this.visit.cache.write = options.cache.write ?? this.visit.cache.write;
+		visit.cache.read = options.cache.read ?? visit.cache.read;
+		visit.cache.write = options.cache.write ?? visit.cache.write;
 	} else if (options.cache !== undefined) {
-		this.visit.cache = { read: !!options.cache, write: !!options.cache };
+		visit.cache = { read: !!options.cache, write: !!options.cache };
 	}
 	// Delete this so that window.fetch doesn't mis-interpret it
 	delete options.cache;
@@ -103,7 +107,7 @@ export async function performNavigation(
 		const pagePromise = this.hooks.call('page:load', { options }, async (visit, args) => {
 			// Read from cache
 			let cachedPage: PageData | undefined;
-			if (this.visit.cache.read) {
+			if (visit.cache.read) {
 				cachedPage = this.cache.get(visit.to.url);
 			}
 
@@ -114,13 +118,10 @@ export async function performNavigation(
 		});
 
 		// Create/update history record if this is not a popstate call or leads to the same URL
-		if (!this.visit.history.popstate) {
+		if (!visit.history.popstate) {
 			// Add the hash directly from the trigger element
-			const newUrl = this.visit.to.url + this.visit.to.hash;
-			if (
-				this.visit.history.action === 'replace' ||
-				this.visit.to.url === this.currentPageUrl
-			) {
+			const newUrl = visit.to.url + visit.to.hash;
+			if (visit.history.action === 'replace' || visit.to.url === this.currentPageUrl) {
 				updateHistoryRecord(newUrl);
 			} else {
 				this.currentHistoryIndex++;
@@ -131,17 +132,22 @@ export async function performNavigation(
 		this.currentPageUrl = getCurrentUrl();
 
 		// Wait for page before starting to animate out?
-		if (this.visit.animation.wait) {
+		if (visit.animation.wait) {
 			const { html } = await pagePromise;
-			this.visit.to.html = html;
+			visit.to.html = html;
 		}
 
 		// Wait for page to load and leave animation to finish
 		const animationPromise = this.animatePageOut();
 		const [page] = await Promise.all([pagePromise, animationPromise]);
 
+		// Abort if another visit was started in the meantime
+		if (visit.id !== this.visit.id) {
+			return;
+		}
+
 		// Render page: replace content and scroll to top/fragment
-		await this.renderPage(this.visit.to.url, page);
+		await this.renderPage(page);
 
 		// Wait for enter animation
 		await this.animatePageIn();
@@ -150,8 +156,8 @@ export async function performNavigation(
 		await this.hooks.call('visit:end', undefined, () => this.classes.clear());
 
 		// Reset visit info after finish?
-		// if (this.visit.to && this.isSameResolvedUrl(this.visit.to.url, requestedUrl)) {
-		// 	this.createVisit({ to: undefined });
+		// if (visit.to && this.isSameResolvedUrl(visit.to.url, requestedUrl)) {
+		// 	this.visit = this.createVisit({ to: undefined });
 		// }
 	} catch (error: unknown) {
 		// Return early if error is undefined (probably aborted preload request)
@@ -164,7 +170,7 @@ export async function performNavigation(
 
 		// Rewrite `skipPopStateHandling` to redirect manually when `history.go` is processed
 		this.options.skipPopStateHandling = () => {
-			window.location.href = this.visit.to.url + this.visit.to.hash;
+			window.location.href = visit.to.url + visit.to.hash;
 			return true;
 		};
 
diff --git a/src/modules/renderPage.ts b/src/modules/renderPage.ts
index d90a64d..50d60a7 100755
--- a/src/modules/renderPage.ts
+++ b/src/modules/renderPage.ts
@@ -4,18 +4,12 @@ import { PageData } from './fetchPage.js';
 
 /**
  * Render the next page: replace the content and update scroll position.
- * @returns Promise<void>
  */
-export const renderPage = async function (this: Swup, requestedUrl: string, page: PageData) {
+export const renderPage = async function (this: Swup, page: PageData): Promise<void> {
 	const { url, html } = page;
 
 	this.classes.remove('is-leaving');
 
-	// do nothing if another page was requested in the meantime
-	if (!this.isSameResolvedUrl(getCurrentUrl(), requestedUrl)) {
-		return;
-	}
-
 	// update state if the url was redirected
 	if (!this.isSameResolvedUrl(getCurrentUrl(), url)) {
 		updateHistoryRecord(url);


Test patch:
diff --git a/cypress/e2e/main.spec.js b/cypress/e2e/main.spec.js
index 475bf82..d6cf490 100644
--- a/cypress/e2e/main.spec.js
+++ b/cypress/e2e/main.spec.js
@@ -389,6 +389,31 @@ describe('Navigation', function () {
 		cy.shouldHaveH1('Page 2');
 	});
 
+	it('should ignore visit if a new visit has started', function() {
+		cy.delayRequest('/page-2.html', 1000);
+		cy.triggerClickOnLink('/page-2.html');
+		cy.wait(50);
+		cy.triggerClickOnLink('/page-3.html');
+		cy.shouldBeAtPage('/page-3.html', 'Page 3');
+		cy.wait(1000);
+		cy.shouldBeAtPage('/page-3.html', 'Page 3');
+	});
+
+	it('should ignore visit if a new visit to same URL has started', function() {
+		const titles = [];
+		this.swup.options.linkToSelf = 'navigate';
+		this.swup.hooks.on('visit:start', (visit) => titles.push(visit.id));
+		this.swup.hooks.on('content:replace', () => document.title = titles[titles.length - 1]);
+
+		cy.delayRequest('/page-2.html', 500);
+		cy.triggerClickOnLink('/page-2.html');
+		cy.wait(50);
+		cy.triggerClickOnLink('/page-2.html');
+		cy.shouldBeAtPage('/page-2.html', titles[1]);
+		cy.wait(500);
+		cy.shouldBeAtPage('/page-2.html', titles[1]);
+	});
+
 	// it('should ignore visit when meta key pressed', function() {
 	//     cy.triggerClickOnLink('/page-2.html', { metaKey: true });
 	//     cy.wait(200);
diff --git a/src/modules/__test__/visit.test.ts b/src/modules/__test__/visit.test.ts
new file mode 100644
index 0000000..96eaab6
--- /dev/null
+++ b/src/modules/__test__/visit.test.ts
@@ -0,0 +1,92 @@
+import { beforeEach, describe, expect, it } from 'vitest';
+import Swup from '../../Swup.js';
+import { Visit, createVisit } from '../Visit.js';
+
+class SwupWithPublicVisitMethods extends Swup {
+	public createVisit = createVisit;
+}
+
+const swup = new SwupWithPublicVisitMethods();
+let visit: Visit;
+
+describe('Visit', () => {
+	beforeEach(() => {
+		visit = swup.createVisit({ to: '' });
+	});
+
+	it('is an object', () => {
+		expect(visit).to.be.an('object');
+	});
+
+	it('has an id', () => {
+		expect(visit.id).to.be.a('number');
+	});
+
+	it('generates unique ids', () => {
+		let id = visit.id;
+		visit = swup.createVisit({ to: '' });
+		expect(visit.id).to.not.equal(id);
+	});
+
+	it('has a from object with the current URL', () => {
+		expect(visit.from).to.be.an('object');
+		expect(visit.from.url).to.be.a('string');
+		visit = swup.createVisit({ to: '', from: '/from' });
+		expect(visit.from).toMatchObject({ url: '/from' });
+	});
+
+	it('has a to object with the next URL', () => {
+		expect(visit.to).to.be.an('object');
+		expect(visit.to.url).to.be.a('string');
+		visit = swup.createVisit({ to: '/to' });
+		expect(visit.to).toMatchObject({ url: '/to' });
+	});
+
+	it('has an animation object', () => {
+		expect(visit.animation).to.be.an('object');
+		expect(visit.animation).toMatchObject({
+			animate: true,
+			name: undefined,
+			scope: swup.options.animationScope,
+			selector: swup.options.animationSelector
+		 });
+	});
+
+	it('has a container array', () => {
+		expect(visit.containers).to.be.an('array');
+		expect(visit.containers).toEqual(swup.options.containers);
+	});
+
+	it('has a trigger object', () => {
+		expect(visit.trigger).to.be.an('object');
+		expect(visit.trigger).toMatchObject({
+			el: undefined,
+			event: undefined
+		 });
+	});
+
+	it('has a cache object', () => {
+		expect(visit.cache).to.be.an('object');
+		expect(visit.cache).toEqual({
+			read: swup.options.cache,
+			write: swup.options.cache
+		});
+	});
+
+	it('has a history object', () => {
+		expect(visit.history).to.be.an('object');
+		expect(visit.history).toEqual({
+			action: 'push',
+			popstate: false,
+			direction: undefined
+		});
+	});
+
+	it('has a scroll object', () => {
+		expect(visit.scroll).to.be.an('object');
+		expect(visit.scroll).toEqual({
+			reset: true,
+			target: undefined
+		});
+	});
+});