roxiness / routify

Automated Svelte routes
https://routify.dev
1.89k stars 87 forks source link

isChangingPage Is Set "Too Late" #421

Open Rican7 opened 2 years ago

Rican7 commented 2 years ago

Description

While trying to use the $isChangingPage helper, I noticed that it fires "too late". It won't set to true until after the page is already navigated to, which makes it relatively useless. 😞

I believe the issue here is that the value isn't set to true until after the route's components have been preloaded...

Would it not make sense to just move the stores.isChangingPage.set(true) bit to before the route.api.preload() call? Or, could it not be maybe moved to be set as a hook in the $beforeUrlChange helper, similar to how its currently set to off in $afterPageLoad?

Package Versions

$ npm ls --depth=0 @roxi/routify svelte
variedvibe.com@1.0.0 REDACTED
├── @roxi/routify@2.18.4
└── svelte@3.44.3
jakobrosenberg commented 2 years ago

Hi @Rican7, moving $isChangingPage would be a breaking change. The helper only shows when a page is actually changing, not when it is loading.

There are a few ways this could be handled:

PR idea 1

We could do something like this instead

    stores.isLoadingPage.set(true)

    //preload components in parallel
    route.api.preload().then(() => {
      stores.isChangingPage.set(true)
      stores.isLoadingPage.set(false)

      //run callback in Router.svelte    
      callback(nodes)
    })
if ($isLoadingPage || $isChangingPage) { ... }

PR idea 2

Another option would be to introduce a new Routify option

    if (options.changingPageIncludesPreload)
      stores.isChangingPage.set(true)

    //preload components in parallel
    route.api.preload().then(() => {
      stores.isChangingPage.set(true)
      //run callback in Router.svelte    
      callback(nodes)
    })

Alternatively

you could also use a combination of beforeUrlChange and afterPageLoad.

import { beforeUrlChange, afterPageLoad } from "@roxi/routify"

let loading = false

$beforeUrlChange(() => {
  loading = true
  return true
})
$afterPageLoad(() => (loading = false))
Rican7 commented 2 years ago

Oh wow! Thanks @jakobrosenberg!

I see how that would be a breaking change that could mess with people's logic. Totally fair.

So yea, either a new helper or a config change. I kind of feel like a new helper would actually be better, as they are semantically different now that I think about it.

For now I've worked around it with a solution similar to what you mentioned:

  import { isChangingPage, beforeUrlChange } from "@roxi/routify";

  // TODO: Remove in favor of just using `$isChangingPage` once issue
  // https://github.com/roxiness/routify/issues/421 is fixed.
  $: isLoading = $isChangingPage;
  $beforeUrlChange(() => (isLoading = true));
jakobrosenberg commented 2 years ago

I really like your solution. 😄

Any ideas for what a new helper should look like?

$: isLoading = $isLoadingPage || $isChangingPage

or just

$: isLoading = $isLoadingPage
Rican7 commented 2 years ago

Thanks!

Hmmm... I actually think the first idea is probably best, as its more explicit: $: isLoading = $isLoadingPage || $isChangingPage.

jakobrosenberg commented 2 years ago

Much agree. Would you be interested in doing a PR? (idea no 1 posted above) I can also do it, but I might not have time till next weekend.

Rican7 commented 2 years ago

I could try and see if I'm able to, yea. 😃