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

🐛 BUG: Turbolinks breaks hydration on repeat page views #3128

Closed
1 task
nonphoto opened this issue Apr 17, 2022 · 12 comments · Fixed by #3283
Closed
1 task

🐛 BUG: Turbolinks breaks hydration on repeat page views #3128

nonphoto opened this issue Apr 17, 2022 · 12 comments · Fixed by #3283
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)

Comments

@nonphoto
Copy link
Contributor

What version of astro are you using?

1.0.0-beta.12

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

pnpm

What operating system are you using?

Mac

Describe the Bug

Turbolinks merges script tags in head and will not run a script more than once, so Astro islands will not hydrate when viewing a page for the second time.

This is only an issue with the Turbolinks integration at the moment. An easy fix would be to add hydration scripts to body instead of head. However, I can imagine other scenarios where it would be useful to insert and remove islands and have hydration handled automatically, so I think using a MutationObserver would be a more general solution.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-kq5z3u?file=src/pages/about.astro

Participation

  • I am willing to submit a pull request for this issue.
@natemoo-re natemoo-re self-assigned this Apr 22, 2022
@natemoo-re
Copy link
Member

There are some fixes in #2899 for this, but I'll need to split them out to a separate PR.

MutationObserver is one option but a CustomEvent to trigger hydration would be more performant.

@nonphoto
Copy link
Contributor Author

nonphoto commented May 4, 2022

@natemoo-re CustomEvent is a good call! Is there an issue somewhere I can follow for this?

Thanks for the Turbolinks fix @tony-sull

@narration-sd
Copy link

narration-sd commented May 6, 2022

@tony-sull Tony, was quite happy to see this of yours; unfortunately it has problems I think with the basic approach. So we may need to see if @natemoo-re has a way that works better.

What's the problem? Your patch seems to work at first, and on a dev system. But when I put the build on a droplet, I got a pretty terrific FOUC -- a strong white flash -- between any of three pages.

I had to think about this a bit, as I was also getting different network requests depending on whether I tested a local dev server, a local build preview, or out in the world on a Netlify instance.

I think it resolves to timing effects in the browser rendering engine, altering the requests it makes and re-renders depending on latency (server speed when local). This would be natural, as browsers are pretty clever about how much presentation effort they make vs. arriving code or scripts and their timing. As with the results when you fire off the scripts, which is the basis of your method in the patch.

I'm attaching a screenshot of a worst case, for the droplet case where there's even a pretty short latency add. Have a look at the network requests. There are actually doubled requests for each page -- I'm only route switching to one, and then two, with no local bounce.

Whether you get doubled, other results I've seen, or on which page depends on the server arrangements above, and some of the browser-side variations depend on what is actually on the page. Some it can be pretty entertaining to think about, but we really can't -- we have to treat the browser rendering engine as a black box, and one that can change.

Then I'm thinking we probably need to see @natemoo-re 's work, and whether that has cleaner results, no?

The FOUC itself is explained in that I'm using pages based on the Astro skeleton included when you initiate a project. One of them just uses a simpler markdown content with the given counter, and the other includes some different Vue components of mine -- I'm trying to model up whether I can replace (with much regret) an interesting website engine presently constructed on Gridsome. Both would run via Netlify.

What is in the boilerplate re-used in those pages (in global.css) is a media queried overlay of the css. The media check is whither the browser thinks it's in dark mode. I would guess that dark mode is pretty far down on the priorities of the rendering engine, (normal and dev releases of Google Chrome), and so it gets significant delay before being enacted. Since it flips color and background color, a full screen refresh is done. Since I'm on dark mode for my eyes, the initial white screen renders earlier, resulting in the flash. The doubled pull may happen because everything hasn't arrived over the wire by the time its needed for this rerender -- my thinking doesn't feel quite grounded here.

What it does seem is that a scheme to fix the render problem should have everything ready on the initial turbolinks-based load, and not add screen instructions later, either css or javascript. So, very interested in what @natemoo-re has been building up, while appreciative and of respect for the way you've done something here -- thanks! 🌱🌿 👍

p.s. In between other things must do this afternoon, had a very brief glimpse at @natemoo-re 's work on this topic. It appears in using a diff engine to make a more internalized SPA approach that might indeed avoid the problems reported, if one were to guess.

I notice that the pull request is on hold, I think because of @FredKSchott 's marking it as in-progress. This may well be wise, given the sorts of issues that can arise without code testing noticing them, as above. Would it be possible or make sense for Nate's work to be provided as an extension, so that it could be released where we can try it without upsetting the central flow of Astro's program? Of course, SPAs are pretty important to that also, at least seems so here :) Cheers, all...

double-do

@natemoo-re natemoo-re reopened this May 10, 2022
@natemoo-re natemoo-re added needs discussion Issue needs to be discussed s2-medium labels May 10, 2022
@FredKSchott FredKSchott added - P4: important Violate documented behavior or significantly impacts performance (priority) and removed needs discussion Issue needs to be discussed labels May 12, 2022
@FredKSchott
Copy link
Member

Discussed with @tony-sull and @natemoo-re and decided it makes sense to deprecate turbolinks since it isn't working properly and doesn't seem like something that we can easily fix. Nate has been working on a first-class "turbo" mode, and our time would be better spent releasing that as an experimental v1.0 feature.

TODO:

@narration-sd
Copy link

narration-sd commented May 13, 2022

@FredKSchott et al, sounds a wise and intriguing course -- @natemoo-re 's gem sounds great, and as @tony-sull has spoken of it to me.

The challenge I was originally looking into involves replacing, with respectful reluctance towards their circumstances, a Gridsome framework we've used.

Over distance of time, though, realized I may not have been speaking straight about how a master-detail could be arranged, as in the usual in-app docs page with topics on the left and relevant explanations on the right. I think we did it actually by eventing betwen two Vue components, and seems that ought to work fine also in Astro. To be tried.

It's interesting though to wonder if Nate's comparator could do 'frame' replacements with any ease, for a SPA effect on related screen areas like this. I think that's what TurboFrames and its successor were aimed at, and don't know how successfully it worked out, but it may be an interesting ability to consider.

Best fortune all ✨

@nonphoto
Copy link
Contributor Author

Nate has been working on a first-class "turbo" mode

Is there branch or RFC for that somewhere? I'm investigating page transitions with astro and that seems useful.

@tony-sull
Copy link
Contributor

Closed by #3383

Turbolinks is no longer under active development. The @astrojs/turbolinks integration has been deprecated.

Looking for an alternative?

Check out swup! If you are using any client:* script for partial hydration, make sure to install the @swup/scripts-plugin to ensure components are rehydrated after a page navigation.

@narration-sd
Copy link

narration-sd commented May 17, 2022 via email

@narration-sd
Copy link

narration-sd commented May 23, 2022

@tony-sull Tony, with usual hard head, I worked up swup and discovered the scripts plugin myself...

Unfortunately, it doesn't do the job. What it does do is fire up the embedded Vuejs components when you refresh the page. But switching between pages using a simple link-routing menu, they stay dead.

I think the issues are kind of laid out in the following link, somebody bright I've run into before. If I haven't imagined wrong, this will get into several issues with Astro's tendencies on the second link.

How to use Swup with VueJs #208

https://discord.com/channels/830184174198718474/872579324446928896/972782055433842698

  • I keep running into, in early days, not being able to access the App that's been created behind Astro integration scenes -- which I at present at least think is necessary to do even this type of accommodation of Swup
  • you'd like not to have to do any of issue 208-style thing, to feel you could freely use Vue etc. inside Astro, a fundamental

Thus unless I've missed something, always possible especially cruising at altitude!, it seems @natemoo-re 's gizmo, outfitted with automatic setup for component inclusions, is likely pretty important for Astro to have.

A Sunday evening's take on it, anyway...

Best,
Clive

@narration-sd
Copy link

narration-sd commented May 25, 2022

@tony-sull, @natemoo-re et al, Just a little more on this picture. I've managed to make Swup work partially by moving its declaration and init up to the Navbar, which is essentially a Layout. But this is very partial, and in fact fails with console logging saying the root of the Layout among others can't be found, 'sometimes'.

The information here, I believe, is that Swup needs to be installed at a higher level in the chain -- at the createApp Vue object as it often has been. I think this will be true for other packages besides Swup, and thus had a thought or two further here, on exposing that, as many have requested.

Is there any reason that the defineConfig call couldn't return an array or better collection, of results from Integrations, labelled per integration, thus enabling finding that Vue object, for example? There could be another layer of indexing should you require different aspects of config to return values, as I'm sure you'd think of.

I have seen Swup now actually returning full Astro pages with no network calls, when it works, as between a page with no Vue objects and one that may have even several. But it fails and lists errors mentioned, as soon as you try to click-route between two Vue-included Astro pages. I haven't gone further to see what happens in any detail with state, either. And I haven't tried to arrrange any Swup events which would probably be needed as well.

In addition, Swup is kind of a pre-present concoction, and may not really be apropos for an Astro w/multicomponents world.

So once again, it seems Nate's engine, and wrapped with enough awareness of what really goes on in Astro to 'just work' without having to code, for heaven's sake, individual event handlers to pick up the slack, would feel a very nice answer for Astro.

All from a distance I admit, having to be more of a 'user' this time, if very encouraging to you guys...

@narration-sd
Copy link

narration-sd commented May 28, 2022

@tony-sull, @natemoo-re et al, a little closure on where I got to on this, in too many 'free hours' spent...as you do, esp. learning quirks of yet another js and build environment, which I am supposed to be retired from...but the practices,, curiosity, and intutiion for the chase apparently haven't abated -- and smiles aside, I wanted to close an investigation for some momentary return of consulting.

  • first, @natemoo-re 's Improve nested and client:only hydration #3455 looks about ready to go just now, and appears a considerable (and necessary) step forward in all of this area
  • then, in stages, I got a variation of your Vue integration building, able to even include swup (!) and operating in developmentv ia yalc, once swups difficulties with module address finally were overcome.
  • played with events to see if I could actually get it to properly function, starting from How to use Swup with VueJs swup/swup#208
  • nothing will do it, and I believe by now that this is because swup in the astro environment isn't able to deal with Vue (3 at least) instances properly.
  • a non-Vue page to a Vue component including page reseen will work, fully instantiiates, no calls to server. But from one Vue page to another, two flag problems occur, which app.unload() or any other ideas were not able to affect:
    a) console log error about needing to do that unload before instantiating another Vue, even though it seemed to be done, and
    b) complaint about not finding root of a component not present but from the preceding page, which is probably because of a) so it's still in play, but not part of the upcoming target astro page.

So, what could one plan to use?

  • on the original reason to try these things, I noticed that the Astro doc, improved over the Snowpack doc, actually does a successful master-detail arrangement as its fundamental layout. It operate reasonably, and if one wanted more smoothness, then back to a dual Vue compenent set, one per frame on a single Astro page. Backing off from perfection, your islands may be up to the jobs imagined, and I could have save a lot of admittedly educational effort...I did of course look at the current docs source to see how simply you'd accomplished this...
  • Nate seems to feel turbolinks-type systems may work with his upcoming PR. That would be great, leading to minimal update 'noise' for the viewer and faster response. Given the above, and its age, I'm not confident how well swup would, at least when differing Vue components were present., and turbolinks is well past being supported. It would be a judgement call whether inviting the extra functonality possible with the supporte derivation of turbolinks would be wise, though you could say what works and doesn't perhaps if it comes to that.
  • given this experience with Astro, I wouldn't foresee difficulty building a site with some quite complex Vue components in it, including Netlify/Fauna globality and some intricate data conversion, as was done in a very short interval for this site in its moment: https://combatcovid.equipment . As just discussed, the Guide part could be much simpler, or use the same Vue all on one page to get full smoothness, so you are giving both opportunities, and this is a marker for other things we might imagine, no?

Ok, and hope the commentary here is useful, makes some worth for all the words spent on this cause, and in the midst of your efforts which I can see now are quite intense. All best, and wishes Astro finds a very nice using community, as seems it ought definitely to do.

Regards and cheers to the team,
Clive

p.s. will include @FredKSchott also, for the compliments to you guys, and his own efforts :)

@narration-sd
Copy link

...edited above a bit for clarity, in case you are reading it from unimproved email...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants