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

isChangingPage Is Set "Too Late" #421

Open
Rican7 opened this issue Jan 2, 2022 · 6 comments
Open

isChangingPage Is Set "Too Late" #421

Rican7 opened this issue Jan 2, 2022 · 6 comments
Labels
bug Something isn't working needs triage v2

Comments

@Rican7
Copy link

Rican7 commented Jan 2, 2022

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
@Rican7 Rican7 added bug Something isn't working needs triage labels Jan 2, 2022
@jakobrosenberg
Copy link
Member

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
Copy link
Author

Rican7 commented Jan 2, 2022

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
Copy link
Member

I really like your solution. 😄

Any ideas for what a new helper should look like?

$: isLoading = $isLoadingPage || $isChangingPage

or just

$: isLoading = $isLoadingPage

@Rican7
Copy link
Author

Rican7 commented Jan 2, 2022

Thanks!

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

@jakobrosenberg
Copy link
Member

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
Copy link
Author

Rican7 commented Jan 2, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage v2
Projects
None yet
Development

No branches or pull requests

2 participants