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

Features are de-inited too late on ajax navigation #6437

Open
fregante opened this issue Mar 30, 2023 · 2 comments
Open

Features are de-inited too late on ajax navigation #6437

fregante opened this issue Mar 30, 2023 · 2 comments
Labels
ajax Temporary label to collect Ajax issues bug

Comments

@fregante
Copy link
Member

fregante commented Mar 30, 2023

Description

Context:

In short, the turbo:render event is triggered when the new content is already on the page and observeSelector might have already found it.

The abort controller should be aborted before the new content is added to the page, without reintroducing the issues that #6267 fixed.

How to fix this issue

Properly detect the change of page, for example by using MutationObserver on turbo-frame instead of the turbo:render event:

document.addEventListener('turbo:render', () => {

How to replicate the issue + URL

Hopefully this isn't visible at the moment, but it can be reproed. Paste this into a feature like quick-new-issue

import onAbort from '../helpers/abort-controller';

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

Then:

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

Expected log

xx: init
xx: found new (100)
xx: deinit
xx: init
xx: found new (100)

Current log:

xx: init
xx: found new (100)
xx: found new (50)
xx: deinit
xx: init
xx: found new (50)

Extension version

23.3.2

Browser(s) used

Chrome

@fregante fregante added bug ajax Temporary label to collect Ajax issues labels Mar 30, 2023
@fregante
Copy link
Member Author

fregante commented Mar 30, 2023

Ajax events can be tracked with:

['turbo:before-cache',
'turbo:before-fetch-request',
'turbo:before-fetch-response',
'turbo:before-render',
'turbo:before-stream-render',
'turbo:before-visit',
'turbo:click',
'turbo:frame-load',
'turbo:frame-render',
'turbo:load',
'turbo:reload',
'turbo:render',
'turbo:submit-end',
'turbo:submit-start',
'turbo:visit',
].map(x => document.addEventListener(x, e => console.log(x, document.querySelector('[data-hovercard-type="pull_request"]') ? '✅PR' : '❌noPR', location.pathname)))

Use:

  1. Visit https://github.com/refined-github/refined-github/issues
  2. Paste the above code in the console
  3. Visit the pull requests tab

The expectation is that there will be a PR on the page only on turbo:render, but in reality it's already there at turbo:frame-render, with no prior "unload" event that we can listen to.

Screenshot

Then with the browser buttons:

  1. Go back
  2. Clear console
  3. Go forward

This works correctly, and it would allow to use before-render as the unload event (except that the URL is /pulls in every event)

Screenshot 3

@fregante
Copy link
Member Author

fregante commented May 9, 2023

I might have found a related issue:

Screenshot 16

Easy repro:

  1. Open PR against non-default branch sandbox#53
  2. Click on the "Files" tab
  3. See conversation-activity-filter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ajax Temporary label to collect Ajax issues bug
Development

No branches or pull requests

1 participant