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

use dynamic routes with wrap #73

Closed
joshua1 opened this issue Dec 27, 2019 · 15 comments · Fixed by #135
Closed

use dynamic routes with wrap #73

joshua1 opened this issue Dec 27, 2019 · 15 comments · Fixed by #135
Labels
enhancement New feature or request ready Change implemented in a dev branch
Milestone

Comments

@joshua1
Copy link

joshua1 commented Dec 27, 2019

@ItalyPaleAle you said in your PR for Wrap that you would make modifications to take a promise as first parameter for wrap to enable dynamic wrap import. Is this possible now. this seems like the only way to use both features (wrap and preconditions alongside dynamic route loading )

@ItalyPaleAle
Copy link
Owner

Still not possible, sorry. I ran into some issues when trying to implement that, and it is in the backlog. PRs are welcome :)

@ItalyPaleAle ItalyPaleAle added the enhancement New feature or request label Mar 8, 2020
@hmmhmmhm
Copy link
Contributor

hmmhmmhm commented Apr 6, 2020

Maybe I can find time to look up about this this this weekend. If I don't work overtime...

@frederikhors
Copy link
Contributor

frederikhors commented Apr 10, 2020

@hmmhmmhm it would be amazing. As writed here: #104:

Looking at yrv router (a good router!) we can see two ways of importing routes:

<Router>
  <Route exact path="/promise" component={import('path/to/other-component.svelte')}/>
  <Route exact path="/lazy" component={() => import('path/to/another-component.svelte')}/>
</Router>

With https://github.com/ItalyPaleAle/svelte-spa-router/blob/master/Advanced%20Usage.md#async-route-loading we have just the lazy one. Am I correct?

I think it would be useful (and amazing) to have both in svelte-spa-router (three-shaked if we do not use them).

The promise way I think is amazing because I can have a lot of routes and maybe I just need to load immediately just the one I need and after that in background start downloading others.

What do you think about?

@hmmhmmhm
Copy link
Contributor

hmmhmmhm commented Apr 12, 2020

@frederikhors What you said is a different issue from wrap.
Since you left an issue in my report, I will answer it here hmmhmmhm/svelte-spa-chunk#3.

@ItalyPaleAle ItalyPaleAle added this to the v3.0 milestone Apr 18, 2020
@ItalyPaleAle
Copy link
Owner

ItalyPaleAle commented Sep 6, 2020

@hmmhmmhm @frederikhors @joshua1 @BogdanDarius (tagging everyone who ever asked about this feature)
sorry this took SO LONG.

I finally managed to get a working prototype of this, in the dynamic-import branch.

It works by creating the wrapAsync method to wrap the route:

import {wrapAsync} from '../../Router.svelte'
export default {
    '/async': wrapAsync(() => import('./Async.svelte')
}

(Note that you must use a function definition, such as () => import('foo') and not import('foo'), which is an invocation)

Then it should just work 😄

The only thing I'd love your input on is on how to handle the "loading" state. Right now, I've created another event called routeLoading that is emitted when the route starts loading. However, the router on its own does not display anything while the route is loading (for now, it just shows the previous route). Sadly, returning a Promise isn't possible.

I have a few options I'm considering:

  1. Make developers listen to the routeLoading and then routeLoaded events to manually handle the state of the route loading asynchronously
  2. The router could export a named slot "loading" that will be used to display temporary content, such as this:
    <Router {routes}>
      <div slot="loading">
          Show something while the route is loading
      </div>
    </Router>
  3. The route definition object could have a new convention for a route named "loading", such as:
    import {wrapAsync} from '../../Router.svelte'
    export default {
        '/async': wrapAsync(() => import('./Async.svelte'),
        '*': NotFound,
        'loading': Loading,
    }

What are your thoughts on the above?

I'm leaning towards option 3 as that seems the most idiomatic to this router. But would like to hear your thoughts.

I'm also open to suggestions on other ways this can be implemented.

@BogdanDarius
Copy link

@hmmhmmhm @frederikhors @joshua1 @BogdanDarius (tagging everyone who ever asked about this feature)
sorry this took SO LONG.

I finally managed to get a working prototype of this, in the dynamic-import branch.

It works by creating the wrapAsync method to wrap the route:

import {wrapAsync} from '../../Router.svelte'
export default {
    '/async': wrapAsync(() => import('./Async.svelte')
}

(Note that you must use a function definition, such as () => import('foo') and not import('foo'), which is an invocation)

Then it should just work

The only thing I'd love your input on is on how to handle the "loading" state. Right now, I've created another event called routeLoading that is emitted when the route starts loading. However, the router on its own does not display anything while the route is loading (for now, it just shows the previous route). Sadly, returning a Promise isn't possible.

I have a few options I'm considering:

  1. Make developers listen to the routeLoading and then routeLoaded events to manually handle the state of the route loading asynchronously
  2. The router could export a named slot "loading" that will be used to display temporary content, such as this:
    <Router {routes}>
      <div slot="loading">
          Show something while the route is loading
      </div>
    </Router>
  3. The route definition object could have a new convention for a route named "loading", such as:
    import {wrapAsync} from '../../Router.svelte'
    export default {
        '/async': wrapAsync(() => import('./Async.svelte'),
        '*': NotFound,
        'loading': Loading,
    }

What are your thoughts on the above?

I'm leaning towards option 3 as that seems the most idiomatic to this router. But would like to hear your thoughts.

I'm also open to suggestions on other ways this can be implemented.

Awesome!
I tested wrapAsync on a bigger project, it works as expected.
Regarding the loading I agree that option 3 seems to most intuitive. I do have a question though, could you setup different loaders for some routes or just a single loader for all routes?

@ItalyPaleAle
Copy link
Owner

ItalyPaleAle commented Sep 6, 2020

Thanks a lot for testing it and confirming it works!

Good point. A different loading view for each component was not a requirement I had considered. But it makes sense.

Doing that would not be possible with the options above (maybe with 1). It would require people to manually determine the loader to show, for example by using the $location store to know what component will be loaded.

The only way to do that in the router would be to use option 5:

  • Adding the loader as an argument in wrapAsync, such as wrapAsync(() => import('foo'), LoaderComponent1)

@BogdanDarius
Copy link

I like the idea of having it as an argument to wrapAsync.

@ItalyPaleAle
Copy link
Owner

Thanks @BogdanDarius for the feedback!

The dynamic-import branch has been updated and now the wrapAsync function accepts a component to display while loading the route. If nothing is passed (or a false-y value), then nothing is displayed.

@hmmhmmhm @frederikhors what do you think?

@BogdanDarius
Copy link

Awesome work.
I tested the updated branch, it works as expected.
I find it very clean to include the loader component in the wrapAsync, that is my take on it.

@frederikhors
Copy link
Contributor

Sorry for my delay in replying.

Great job, @ItalyPaleAle, as always. I haven't tested it yet because I don't have time right now but I trust and know it will work.

Personally I agree with the idea number 5 (passing the loading component as an argument of wrapAsync) because very often we need to have different loaders based on what we are loading, but I also need to pass props to it.

Example:

const routes = new Map()
  .set('/home', PageHome)
  .set('/async', wrapAsync(() => import('Players.svelte'), LoaderComponent, {loader: props}))

Can we do something like that?

@ItalyPaleAle
Copy link
Owner

@frederikhors thanks for the feedback!

The problem with the syntax you're suggesting is that the wrapAsync's function signature is based on the wrap's one: wrap(route, userData, ...conditions). userData is an object too, so if we need to pass another object for the props for the loader component, that would be an issue...

@frederikhors
Copy link
Contributor

frederikhors commented Sep 10, 2020

Can we use an obj like:

{ LoaderComponent, props }

?

Like:

const routes = new Map()
  .set('/home', PageHome)
  .set('/async', wrapAsync(() => import('Players.svelte'), { LoaderComponent, props })

@ItalyPaleAle
Copy link
Owner

It would have to be more like {component: LoaderComponent, props} but could work. Let me look into that, and if it's possible to support both that and just LoaderComponent.

As an alternative, because the signature is getting complicated enough already, I could simply make wrap and wrapAsync accept an object with parameters. That would also be more future-proof, and I'm more leaning towards that.

@ItalyPaleAle
Copy link
Owner

I've updated the dynamic-import branch and now there's a single wrap function exposed in svelte-spa-router/wrap which takes an object as argument, and allows dynamically-imported and not routes. 🎉

(The wrap function exported by svelte-spa-router is still available as an interface for backwards compatibility, but now marked as deprecated, and I'll remove it from 4.0)

Thanks a lot for your feedback @frederikhors and @BogdanDarius. I think this will work and I'll merge it into the 3.0-wip branch.

This will be the major new release for the router v3, which I plan to finish ASAP :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready Change implemented in a dev branch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants