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

Passing additional Props to component #131

Closed
matt-psaltis opened this issue Aug 16, 2020 · 6 comments · Fixed by #135
Closed

Passing additional Props to component #131

matt-psaltis opened this issue Aug 16, 2020 · 6 comments · Fixed by #135
Labels
ready Change implemented in a dev branch
Milestone

Comments

@matt-psaltis
Copy link

Thank you for making the only Svelte router that has just worked. A huge bonus - it matches the documentation! :D

I was hoping for a change to the Router component to be able to pass additional props to the rendered component.

Given a router definition the inclusion of the {props} attribute would allow arbitrary data to be passed to the rendered component.

const props = { id: "1234" };

<Router {routes} {prefix} {props} />

Changes required:

1. Include new exported member: export let props; (similar to routes, prefix)
2. Include the {...props} into the component declaration
    {#if componentParams}
      <svelte:component this="{component}" params="{componentParams}" on:routeEvent {...props} />
    {:else}
      <svelte:component this="{component}" on:routeEvent {...props} />
    {/if}

Rendered component can now access these props in the standard way using export let id;

As per: https://svelte.dev/repl/74593f36569a4c268d8a6ab277db34b5?version=3.12.1

Appreciate your consideration on this additional feature.

@ItalyPaleAle
Copy link
Owner

Hi @SaltyDH, thanks a lot for the nice words and for the suggested feature!

As others have seen me write in other PRs, one of the main goals of svelte-spa-router is to keep the module as simple as possible. I've resisted feature creeping many times before because I think that a smaller router would have lots of benefits, including:

  1. Smaller footprint at runtime. At the moment, adding svelte-spa-router adds just about 2KB (uncompressed) to your app's bundle. Because this code is used on the frontend, and shipped to browsers, size matters.
  2. Smaller learning curve. The documentation for this router fits in a single README file (plus an additional "Advanced" ones, which has mostly tips-and-tricks), so it's simpler for people to learn and use
  3. Easier to maintain and less surface for bugs

My rule of thumb is that a feature will be included in the router if and only if one of the two applies:

  • The same goal cannot be accomplished in any other way in the user's code
  • Even if the same goal can be accomplished in the user's code, implementing this on the router would simplify the amount of code users need to write significantly, and it's something that impacts a large enough number of users

Looking at your proposed code change, it seems that what you're trying to accomplish is to be able to pass props from the App component down to individual routes. This sounds like an issue lots of people could face.

After thinking a bit about it, the same thing could also be achieved using Svelte's stores (or contexts).

However, I do want to understand if you've already considered the options above, and if they would not work for your case. Are there benefits in using this approach that should cause us to consider adding this feature as opposed to using an alternative option?

@matt-psaltis
Copy link
Author

Hi @ItalyPaleAle thank you for taking the time to consider this proposal. I completely understand where you're coming from and it was part of my thought process in putting together what I hoped was a minimal and considered change to the router. Let me see if I can distil my thoughts and use case down for your consideration.

A top level router acts as a seam between logically disparate portions of the application. Typically this separation is desired because it separates routes such as /tasks from /users, two logically disconnected parts of the application. When using the nested routers feature that this router supports, this paradigm is the polar opposite, two logically connected components that are divided by a seam (the router) e.g. /tasks/:taskId/overview and /tasks/:taskId/details. To me, it makes a lot of sense to use the router in this situation because of the features provided by the router including consistent url history, and synchronisation with the address bar etc, guard etc.

In terms of stores and contexts:

  1. The application data is in fact stored in a store. This store is partitioned by taskId and so one of the additional details I need to pass across the seam from parent component -> (router) -> child component is the taskId. Furthermore, data fetching occurs in the parent component at the /tasks/:taskId route so that each of the sub routes (/overview, /details for example) do not need to fetch additional information. The problem here is that because the parent component's route is /tasks/:taskId, this voids the ability to use params to pass the taskId value across the seam created by the router to the child components.
  2. Contexts definitely make it possible but we go from export let taskId to the contexts API. My belief is we're getting away from simplicity, increasing the state management in every child component to be able to retrieve the task information.

In contrast, the suggestion I have put forward:

  1. Uses an intrinsic feature of Svelte and therefore whilst a new capability of the router, is not a new technical feature to implement, minimising technical debt to the project (speaks to point 1 - Smaller footprint at runtime).
  2. Uses the same data passing technique as the params variable - the only difference is the data structure of variable.
  3. Minimises developer's surprise (speaks to point 2 - Smaller learning curve) when working with the router. They're used to working this way with normal Svelte component hierarchies why should the router behave differently?
  4. Using https://svelte.dev/repl/74593f36569a4c268d8a6ab277db34b5?version=3.12.1 as a basis, the removal of the {...props} attribute from the svelte:component reduces the overall character size by 455 characters (that's the raw, unminified, uncompressed change) so I agree there is a footprint change associated with this proposal. If any of the above has swayed you to further investigation, I'm happy to provide gzip comparisons or similar?

Hopefully my thoughts resonate with you and you may consider these suggestions for a future update.

Kind regards.

@ItalyPaleAle
Copy link
Owner

Sorry for the delay in responding…

Thanks for your thoughtful response. I agree with your point and I'm going to accept this.

Would you like to open a PR (to get proper credits)?

ItalyPaleAle added a commit that referenced this issue Sep 19, 2020
@ItalyPaleAle ItalyPaleAle added the ready Change implemented in a dev branch label Sep 19, 2020
@ItalyPaleAle
Copy link
Owner

@matt-psaltis this is implemented in the 3.0 branch and will be released shortly. Thanks for the contribution!

One note: because the props are passed to every single route, if the route doesn't ask for them, you'd see a warning similar to <Name> was created with unknown prop 'foo' (see #98)

@ItalyPaleAle ItalyPaleAle added this to the v3.0 milestone Sep 19, 2020
@ItalyPaleAle ItalyPaleAle mentioned this issue Sep 19, 2020
2 tasks
@matt-psaltis
Copy link
Author

Hi @ItalyPaleAle sorry I've been unresponsive to this I would have liked to contribute with the pull request, thank you for doing so on my behalf.

With this change, it looks like the props are exposed as part of the routes configuration and I've successfully been able to add them via routes. One difference in the original suggestion was having the props variable exported so that it can be supplied directly on the router component from the parent component rather than via the routes config. Just curious if you can spare a moment :) thanks again!

@ItalyPaleAle
Copy link
Owner

Hey @matt-psaltis thanks again for the contribution and for the convincing argument above!

I initially merged the change just as you proposed above. However, after some testing, I decided to alter the design for two reasons.

  1. The first is that I know that once I "opened the floodgates", users would have asked for that, so might as well just do it right away and offer the most flexible option. The reason why I think that is because at the same time as I was merging your code change, I was working on use dynamic routes with wrap #73 (support for dynamically-imported components). My initial design had a lot of proposals that were "less flexible", such as having a single component to show as placeholder while routes were loading, for all routes. Users' feedback was that they preferred a way to have a different loading component for each route, to be set in the wrap method. So, I assumed that similar conclusions could be made for this code change too.
  2. The second reason is related to issue again: <component> was created with unknown prop 'params' #98. Assume the router passes the prop foo to the component Bar. If Bar doesn't have that prop (ie., Bar doesn't say export let foo), then you get a runtime warning while in development mode. This is just a warning, and doesn't appear when compiling in production mode, but it has gotten users annoyed. There's an issue open with Svelte upstream, but they don't seem too interested in fixing it. So, I didn't want to create even more situations when users could see runtime warnings.

I know that the new API design is a bit more complex than just passing a prop to the router component... But I hope that the gains in flexibility (and the less annoying warnings) will compensate for the minor additional annoyance of having to use wrap.

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

Successfully merging a pull request may close this issue.

2 participants