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 ajax navigation detection #5720

Merged
merged 5 commits into from
Jun 24, 2022
Merged

Fix ajax navigation detection #5720

merged 5 commits into from
Jun 24, 2022

Conversation

fregante
Copy link
Member

@refined-github/maintainers please test because I'd like to release a new version within a few hours

Screen.Recording.2.mov

@fregante
Copy link
Member Author

For debugging, run this in the console:

monitorEvents(document.documentElement, ['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'])

Click example

Screen Shot 2

Back button example

Screen Shot 3

@FloEdelmann
Copy link
Member

The old event names are probably still used in GHE, so we should keep supporting them for a while, shouldn't we?

@fregante fregante merged commit f11c406 into main Jun 24, 2022
@fregante fregante deleted the ajax-again branch June 24, 2022 16:33
@cheap-glitch
Copy link
Member

cheap-glitch commented Jun 24, 2022

Seems like a lot of features are still affected by this, e.g.

  • highlight-collaborators-and-own-conversations
  • clean-rich-text-editor
  • clean-pinned-issues
  • and probably others.

It's likely the de-duplication mechanism (with the has-rgh and has-rgh-inner) needs to be rethought a bit.

(This is on 22.6.24 btw.)

@fregante
Copy link
Member Author

fregante commented Jun 25, 2022

All of them work for me on Chrome (latest main build):

Screen.Recording.mov

@cheap-glitch
Copy link
Member

cheap-glitch commented Jun 25, 2022

Okay, so there must be some kind of tomfoolery goin' on here, because I still see the old event names on Firefox:

image

So basically f11c406 broke everything for me 😞

Could it be GitHub doing some A/B testing or something?


Edit: as a temporary fix we could remove the condition here

f11c406#diff-cb1dafea7a9c68c5c2ebb2d56f749b3506c13f7d26feeaf56c86521b25c60358R144-R146

and just apply the polyfill indiscriminately.

@fregante
Copy link
Member Author

fregante commented Jun 25, 2022

Likely AB testing. I made the change you suggested and released yet another hotfix release 😁
We haven't had to do this in years!

I'm glad I wrote the code this way instead of a generic "if GHE, use the old events;" it would have been more difficult to push another release.

@cheap-glitch
Copy link
Member

I made the change you suggested and released yet another hotfix release

Working great now, thanks!

@yakov116
Copy link
Member

Stopped working for me now...

@cheap-glitch
Copy link
Member

cheap-glitch commented Jun 28, 2022

Stopped working for me now...

Same, it broke sometimes yesterday. Looks like everyone has the new events now:

image

But that also means the polyfill actually breaks ajax detection…

Edit: maybe because the polyfill dispatches the "turbo" events on the document element when they should be sent on window directly?

@fregante
Copy link
Member Author

fregante commented Jun 28, 2022

You're misunderstanding how the polyfill works.

All of our event listeners are now for the turbo events. The listeners themselves are never affected by the polyfill, no events are prevented nor filtered. Any native turbo events just go through.

The polyfill only catches pjax events and then dispatches them for own listeners.

If everyone has the latest events, then the polyfill isn't being triggered at all. The only thing that might be broken is the automatic dismissal of the polyfill, which again only redirects pjax events (no pjax means no polyfilled events)


Now, if they changed their dispatch location, then we also need to update the listeners, but the polyfill isn't part of the issue.

RGH 22.6.25 works for me though, every browser

@cheap-glitch
Copy link
Member

Then I guess it's a deduplication issue? 🤔 I'm seeing #5738 too

@fregante
Copy link
Member Author

fregante commented Jun 29, 2022

#5738 is being fixed in #5742, but that's an issue on the first load; #5719 is about subsequent ajax navigation.

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

Successfully merging this pull request may close these issues.

Nothing works until you refresh the page (Ajax issue)
4 participants