Skip to content

Detect page change earlier, remove listeners#7513

Merged
fregante merged 7 commits into
mainfrom
disable-features-sooner
Jul 10, 2024
Merged

Detect page change earlier, remove listeners#7513
fregante merged 7 commits into
mainfrom
disable-features-sooner

Conversation

@fregante

@fregante fregante commented Jun 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Repro

Paste this in a feature file, like quick-new-issue

void features.add(import.meta.url, {
	init(signal) {
		console.log('xx: init');
		signal.addEventListener('abort', () => {
			console.log('xx: deinit');
		});
		observe('a', () => {
			console.log('xx: found new');
		}, {signal});
	},
});

Then:

  1. Visit any repo page
  2. Click on another tab

Before

Notice the "found new" observer firing before the deinit listener.

3

After

  1. "deinit" first
  2. "found new" later

2

@fregante
fregante marked this pull request as ready for review July 9, 2024 07:29
@fregante

fregante commented Jul 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

This appears to work without visible side effects 😮

If #5723 is ever implemented, then we'll have to revisit, but I don't see that happening anytime soon.

Tracking issue:

@fregante
fregante requested a review from kidonng July 9, 2024 08:44
@fregante fregante mentioned this pull request Jul 9, 2024
Closed
@kidonng

kidonng commented Jul 10, 2024

Copy link
Copy Markdown
Member

This appears to work without visible side effects 😮

Sadly I don't have much opportunity to test this (and follow all the pjax/turbo changes in recent years), if it works then 🎉

@fregante
fregante merged commit 52d4ee9 into main Jul 10, 2024
@fregante fregante changed the title De-init features earlier Detect page change earlier, remove listeners Jul 10, 2024
@fregante
fregante deleted the disable-features-sooner branch July 10, 2024 09:06
@fregante fregante mentioned this pull request Jul 23, 2024
Closed
@fregante fregante mentioned this pull request Jul 20, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kidonng kidonng Awaiting requested review from kidonng

Assignees

No one assigned

Labels

bug

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Features are de-inited too late on ajax navigation

2 participants

@fregante @kidonng