fix(nuxt): stop loading indicator if page keys are the same

[huang-julien]

🔗 Linked issue

fix #24928
fix #25039

❓ Type of change

• 📖 Documentation (updates to the documentation, readme or JSdoc annotations)
• 🐞 Bug fix (a non-breaking change that fixes an issue)
• 👌 Enhancement (improving an existing functionality like performance)
• ✨ New feature (a non-breaking change that adds functionality)
• 🧹 Chore (updates to the build process or auxiliary tools and libraries)
• ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Hi 👋 this PR stops the loading indicator by triggering page:loading:end if page keys are the same when re-rendering

📝 Checklist

• I have linked an issue or discussion.
• I have updated the documentation accordingly.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[danielroe]

should we use isChangingPage?

[huang-julien]

I don't know 🤔 , isChangingPage is relying on to and from from a router guard.

This implementation is relying on whether we force rerender the page or not by comparing the generated key.

Do you think we should use a router guard instead ?

[manniL]

Do we want to have a different behavior compared to isChangingPage for checking? If not, let's use it.

[huang-julien]

I think we want to stop the loading indicator only if the setup of the new page won't be run.

Using a router guard with beforeResolve don't necessaraly means that the setup will be re-run 🤔.

[danielroe]

Is there a way to add some tests for loading indicator behaviour and these loading hooks? (Ensuring they are called in the right order, not over-called, etc.)

[Atinux]

We also have the issue on https://nuxt.com/modules when clicking on the categories:

https://volta.s3.fr-par.scw.cloud/Clean_Shot_2024_01_02_at_13_24_58_71ddda020b.mp4

(made this temporary fix: nuxt/nuxt.com@8638e00)

[huang-julien]

I'm trying to add some tests, but we'd need to at least wait for 300ms to get the failing case on the current main branch.

300ms seems to be way too long for the CI and i can't reduce it otherwise tests are becoming false positive.

just FYI, this PR also fix the issue with queries

[manniL]

@huang-julien FYI - we should also check nested page behavior with the back button. Ideally this PR solves that too (see #25039)

[danielroe]

@huang-julien Do you think we could reduce that by updating loading indicator props, e.g. throttle and duration?

[huang-julien]

@huang-julien FYI - we should also check nested page behavior with the back button. Ideally this PR solves that too (see #25039)

Yes... this does also stop the loading indicator in nested pages since the higher one will trigger the stop hook...

@huang-julien Do you think we could reduce that by updating loading indicator props, e.g. throttle and duration?

Sadly it does not work 😢

I have some fixtures ready for tests... but the browser used by playwright (is it chrome ?) seems to be veryyyy slow so we need to wait for 300ms before evaluating the opacity of the indicator

[thezumrad]

My temporary solution is this:
Just hiding it for a certain page

onMounted(() => {
setTimeout(() => {
const loadingIndicator = document.querySelector('.nuxt-loading-indicator');
if (loadingIndicator) {
loadingIndicator.classList.add('!opacity-0')
}
}, 1000);
})

onBeforeUnmount(() => {
const loadingIndicator = document.querySelector('.nuxt-loading-indicator');
if (loadingIndicator) {
loadingIndicator.classList.remove('!opacity-0')
}
})

[manniL]

@thezumrad it should already work in 3.9.1 ☺️

[thezumrad]

Ahah yeah
I've 3.9.0. Did not pay attention

Thanks)