ItalyPaleAle / svelte-spa-router

Router for SPAs using Svelte 3
MIT License
1.53k stars 105 forks source link

feat: add dynamic props #283

Open yaameen opened 1 year ago

yaameen commented 1 year ago

It lets you pass dynamic props to the loaded component. It adheres to the routing cycle and renders components only after the props are resolved. It raises propsFailed event for each prop when the prop fails to resolve.

Follows how you would pass dynamic props to your component.

'/foo': wrap({
    component: Foo,
    props: {
        staticProp: 'this is static',
        dynamicProp: async () => Promise.resolve(`this is dynamic - ${new Date}`),
        anotherProps: () => `Just works!`
    }
 }),
yaameen commented 1 year ago

Following is an example that uses await.

'/foo': wrap({
    component: Foo,
    props: {
        staticProp: 'this is static',
        dynamicProp: async () => Promise.resolve(`this is dynamic - ${new Date}`),
        anotherProps: () => `Just works!`,
        delayed: async () => await fetch(`https://api.chucknorris.io/jokes/random`).then(i => i.json()).then(i => i.value),
    }
 }),
yaameen commented 1 year ago

@ItalyPaleAle any updates?

ItalyPaleAle commented 1 year ago

@yaameen Apologies for the delay, had a crazy week :)

I have reviewed the PR once again and pushed a small commit to your branch to fix some formatting.

There are however 2 things that make me nervous about merging this and I'd like to ask if you could please do more testing (incl. perhaps writing unit tests) in that regards:

  1. props (current line) used to be set outside of the if (componentObj != obj) block, which means that pushing a new route with the same component, but different props, would have worked (the props would have been updated, but the component would not have been re-mounted). You moved that into the if (componentObj != obj) block, and I'm worried this may be a breaking change. Is there a reason why the block setting props needs to be within the if?
  2. Another thing that's concerning to me is that before the props object was always assigned synchronously. Now, it's always assigned asynchronously: even when props are static, they are wrapped inside a Promise.resolve. This means that they're always executed in the next tick, and I'm not sure what kind of consequences this could bring.
yaameen commented 1 year ago

@yaameen Apologies for the delay, had a crazy week :)

I have reviewed the PR once again and pushed a small commit to your branch to fix some formatting.

There are however 2 things that make me nervous about merging this and I'd like to ask if you could please do more testing (incl. perhaps writing unit tests) in that regards:

  1. props (current line) used to be set outside of the if (componentObj != obj) block, which means that pushing a new route with the same component, but different props, would have worked (the props would have been updated, but the component would not have been re-mounted). You moved that into the if (componentObj != obj) block, and I'm worried this may be a breaking change. Is there a reason why the block setting props needs to be within the if?

Actually it was a mistake putting the prop resolution with in the if block. Also speaking of retaining the component I am failing to produce any occasion where the component is the same but it is not re-mounted when path changes (wild card routes are an exception here). Refer to the following repl.

https://svelte.dev/repl/0d1885829adf467faa5cd432d766a43a?version=3.53.1

Also in the following snippet where obj is a promise https://github.com/ItalyPaleAle/svelte-spa-router/blob/86c99fe7ab4a2810fb5e918c06bfe8df6cade1b9/Router.svelte#L539 like so https://github.com/ItalyPaleAle/svelte-spa-router/blob/86c99fe7ab4a2810fb5e918c06bfe8df6cade1b9/wrap.js#L55 I was wondering is that the reason the component is always re-mounted? Or am I missing something here?

  1. Another thing that's concerning to me is that before the props object was always assigned synchronously. Now, it's always assigned asynchronously: even when props are static, they are wrapped inside a Promise.resolve. This means that they're always executed in the next tick, and I'm not sure what kind of consequences this could bring.

Than you for the feedback, I have simplified the prop resolution where function callback are involved only where required.

yaameen commented 1 year ago

Any update @ItalyPaleAle on this?