craftedbygc / taxi

Taxi is a 🤏 small and 🐊 snappy js library for adding slick PJAX navigation and beautiful transitions to your website.
https://taxi.js.org
565 stars 11 forks source link

Prefetched 404 breaks navigation #21

Closed wiebekaai closed 6 months ago

wiebekaai commented 6 months ago

When prefetching 404s using my configuration, it throws a JavaScript error without displaying the 404 page to the user.

To Reproduce Steps to reproduce the behavior:

  1. Configure transition with removeOldContent: false and bypassCache: true (I remove old content in onEnterCompleted() of my renderer).
  2. Hover a 404 link.
  3. Click the link.
  4. The onLeave transition is triggered, but it does not navigate to the 404 page.

This is how I remove old content:

[...this.wrapper.querySelectorAll('[data-taxi-view]')].filter((e) => e !== this.content).forEach((e) => e.remove());

Info

Screen recording 2024-03-17 at 13 12 18@2x

jakewhiteley commented 6 months ago

Can you confirm if this only happens with removeOldContent: false?

The console error is on purpose, but it should still navigate.

jakewhiteley commented 6 months ago

And the other question - is there any content on the 404 page (a 404 handler with a [data-taxi-view] present), or is it literally a 404 with no body returned?

Edit: So what should happen upon navigating to a 404 - regardless of content on the 404 response - is taxi just defaults to standard browser navigation: the whole page just navigates to the new URL without going through Taxi at all.

I think I would prefer it to check if the new URL has a [data-taxi-view] present, and if so then navigate via PJAX to that URL, and if the response is empty then do a standard browser navigation. Thoughts?

wiebekaai commented 6 months ago

Hi Jake! Hmm, that seems like a good approach already, I just don't know why it's not working for me.

I tried on a clean setup and it works https://github.com/wiebekaai/taxijs-404s. However, on our production site https://askphill.com/pages/services, it plays the leave animation and gets stuck. It does have the proper data attributes.

You can test by replacing a url with some nonsense: Screen recording 2024-03-18 at 23 10 37@2x

Even when I comment out all the code I have in the renderer + transition except for calling done() the issue persists. Or with:

new Core({
  removeOldContent: false,
  bypassCache: true,
});
jakewhiteley commented 6 months ago

I will have to investigate further - I just tried on https://askphill.com/pages/services, and it navigated to the page using default browser navigation without getting stuck. Is it browser specific?

wiebekaai commented 6 months ago

What the hell. Having a hard time reproducing it now. I'll play around a bit and get back to you.

jakewhiteley commented 6 months ago

I've managed to reproduce. I'll come back to you when I know what's up!

jakewhiteley commented 6 months ago

@wiebekaai Coul you update to 1.5.3 and see if its still happening?

wiebekaai commented 6 months ago

@wiebekaai Coul you update to 1.5.3 and see if its still happening?

Wow. Will do right now! Thanks so much 🙏 .

How did you reproduce it?

wiebekaai commented 6 months ago

It runs perfectly on the share theme link and production (we're using Shopify).

The local one has issues, which is less of a priority of course. Seems like the Shopify tracking pixels script does something with the fetch requests.

Screen recording 2024-03-20 at 21 45 18@2x

Screen recording 2024-03-20 at 21 43 46@2x

jakewhiteley commented 6 months ago

Open a new bug for the tracking pixel issue and I'll see if I can help.

The issue for this ticket was upon clicking the link, the focus event is triggered on mouse down, click on mouse up. This meant that the preload was firing ms before the actual fetch.

Normally this is no issue as if theres a cached versino of the target we jsut return that, if theres an in progress promise to fetch the page, we just return that. The fact this was 404ing meant that the preload promise never resolved correctly, causing beforeFetch to fire and nothing else.

I just caught 404 error properly and made it default to browser navigation fallback.

I like the transitions on your site btw! Can link to it in a taxi showcase?

wiebekaai commented 6 months ago

That's interesting, cool to see how this thing works. I'll open a new bug.

Yesssss, would be an honor :)!