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

fix: <Route> received an unexpected slot "default" #3115

Merged
merged 2 commits into from Dec 28, 2021

Conversation

bfanger
Copy link
Contributor

@bfanger bfanger commented Dec 27, 2021

Previously:

<Route>
  {#if subroute}
    <Subroute />
  {/if}
</Route>

if the subroute is falsy an empty default slot is passed which causes the warning. #981

This change renders:

{#if subroute}
  <Route>
    <Subroute />
  </Route>
{:else}
  <Route />
{/if}

Caveat:.
Svelte as has no VirtualDOM, so when a route toggles between having a subroute and not having subroute the component will be remounted and internal state is lost. This is could be a non-issue, as I was not able to produce that scenario.

@changeset-bot
Copy link

changeset-bot bot commented Dec 27, 2021

🦋 Changeset detected

Latest commit: a75eda8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines +133 to +139
{#if components[${l + 1}]}
<svelte:component this={components[${l}]} {...(props_${l} || {})}>
${pyramid.replace(/\n/g, '\n\t\t\t\t\t')}
{/if}
</svelte:component>
</svelte:component>
{:else}
<svelte:component this={components[${l}]} {...(props_${l} || {})} />
{/if}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used to do this trick in my other projects, but a caveat I found is that this would also rerender the component if only the child components change, which is sometimes undesirable, especially for route transitions. And so, I think we should fix the warning in Svelte side, and leave the code as is for now to prevent a potential breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does that happen? Are there situations where the upper level component stays the same but the underlying level changes to falsy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also curious about this — if we can fix it here rather than in Svelte, that would be preferable I think, since the warning is useful.

The components 'branch' is an array of nodes, where every node except the leaf is a layout. Layouts always have a slotted child, leaves never do. Which means that I don't think there's ever a scenario in which a component could move between the consequent and the alternate — layouts will always be in the consequent, leaves will always be in the alternate.

In other words this is a brilliantly simple (and in hindsight, obvious!) fix for this issue as far as I can see — thank you @bfanger! But I'd love to better understand @bluwy's concern before merging.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks for the explanation. I wasn't sure what the components array would contain, but if the quirk I mention never happens. This looks good to me!

@bluwy bluwy added the router label Dec 28, 2021
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants