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

chore: make if blocks tree-shakable #14549

Merged
merged 6 commits into from
Dec 4, 2024
Merged

chore: make if blocks tree-shakable #14549

merged 6 commits into from
Dec 4, 2024

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Dec 4, 2024

Closes #12833.

Thanks to @adiguba for the idea as shown in #13313. This PR aims to make {#if} blocks tree-shakable without any runtime performance regressions.

The principle is very similar to #13313, but executed very differently to ensure that the following remain true:

  • We don't have to change any existing tests and there should be no observable behaviour difference or breaking changes
  • We don't cause runtime overhead and too much code size explosion
  • We allow tree-shaking of if block conditions

The result of these things, means that this PR compiles this:

{#if false}
	Hello world
{:else}
	LOL!
{/if}

In to this:

var consequent = ($$anchor) => {
  var text = $.text("Hello world");

  $.append($$anchor, text);
};

var alternate = ($$anchor) => {
  var text_1 = $.text("LOL!");

  $.append($$anchor, text_1);
};

$.if(node, ($$render) => {
  if (false) $$render(consequent); else $$render(alternate, false);
});

You'll notice that we now have a simple if block inside the $.if that can easily be tree-shaken. Furthermore, unlike #13313, we don't return an array with the path index and a closure, but instead invoke $$render which does a similar thing. Explained in more detail:

  • Invoking the function with the index rather than returning an array is significantly faster in my testing and results in quite a few bytes less code in the minified output.
  • We hoist out the closure rather than have it inline, as in my testing, this had a noticeable impact on performance – which makes sense, given that the closure from the $.if block will be invoked even if the condition hasn't changed – so creating a new closure each time is just utterly wasteful.
  • If there is no branch for something, or a branch gets treeshaken out, then the runtime understands this and fallsback to the default use-case, ensuring things work as expected, including on hydration.

Another difference to #13313 is that we keep the same heuristics as we have now when it comes to "else if" cases, and nest them on the else case, rather than as flattened else if cases within the same $.if block as shown in #13313. Flattening causes tests to fail and also had the side-effect of adding a significant amount of complexity to the codebase.

The nice thing about this PR is that very little changes. However, we can revisit this in the future if we decide it's worth it. In many ways, doing these two workloads separately might actually make more sense to happen in different PRs anyway, as it will be easier to review and revert if something goes wrong that isn't covered by tests.

Copy link

changeset-bot bot commented Dec 4, 2024

🦋 Changeset detected

Latest commit: d2ecf58

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

This PR includes changesets to release 1 package
Name Type
svelte 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

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14549-svelte.vercel.app/

this is an automated message

Copy link
Contributor

github-actions bot commented Dec 4, 2024

Playground

pnpm add https://pkg.pr.new/svelte@14549

@trueadm
Copy link
Contributor Author

trueadm commented Dec 4, 2024

@adiguba Can you add a comment to this PR so you can get co-authoring credit? There's no way for me to manually do it.

@paoloricciuti
Copy link
Member

@adiguba Can you add a comment to this PR so you can get co-authoring credit? There's no way for me to manually do it.

I think you can add Co-Authored: email to the commit message to do it

@adiguba
Copy link
Contributor

adiguba commented Dec 4, 2024

Nice to see this PR 😉
Thanks

@trueadm
Copy link
Contributor Author

trueadm commented Dec 4, 2024

Nice to see this PR 😉 Thanks

I meant to comment on a code suggestion adding a comment to one of the changes sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's the good place for the comment.
Otherwise it's not a big deal... Just merge it !

@Rich-Harris
Copy link
Member

For readability, in cases where we have multiple if blocks...

{#if true}
  <p>yes</p>
{:else}
  <p>no</p>
{/if}

{#if true}
  <p>yes</p>
{:else}
  <p>no</p>
{/if}

...would it make sense to put everything in a single BlockStatement?

var node = $.first_child(fragment);

{
  var consequent = ($$anchor) => {
    var p = root_1();

    $.append($$anchor, p);
  };

  var alternate = ($$anchor) => {
    var p_1 = root_2();

    $.append($$anchor, p_1);
  };

  $.if(node, ($$branch) => {
    if (true) $$branch(0, consequent); else $$branch(1, alternate);
  });
}

var node_1 = $.sibling(node, 2);

{
  var consequent_1 = ($$anchor) => {
    var p_2 = root_3();

    $.append($$anchor, p_2);
  };

  var alternate_1 = ($$anchor) => {
    var p_3 = root_4();

    $.append($$anchor, p_3);
  };

  $.if(node_1, ($$branch) => {
    if (true) $$branch(0, consequent_1); else $$branch(1, alternate_1);
  });
}

A minifier will get rid of the extra {/} characters.

@trueadm
Copy link
Contributor Author

trueadm commented Dec 4, 2024

Thanks for the feedback @Rich-Harris, addressed.

@trueadm trueadm merged commit a220330 into main Dec 4, 2024
11 checks passed
@trueadm trueadm deleted the dce-if-block branch December 4, 2024 18:15
@github-actions github-actions bot mentioned this pull request Dec 4, 2024
@LeonHeidelbach
Copy link

@Rich-Harris The changes introduced in svelte@5.6.2 cause the following problems in one of my code bases when including the browser variable from $app/environment in an if-block.

I have the following code:

{#if !browser}
    <Spinner size="10" />
{/if}

Starting form svelte@5.6.2 this now causes the following error on the client (no errors on the server):

image

The error only occurs, when trying to negate the browser variable inside of an if-block. When trying to basically do the same thing like this the error does not occur:

{#if browser}
    <span>Some placeholder text</span>
{:else}
    <Spinner size="10" />
{/if}

@trueadm
Copy link
Contributor Author

trueadm commented Dec 6, 2024

@LeonHeidelbach I'm looking into this now.

@knd775
Copy link

knd775 commented Dec 6, 2024

@LeonHeidelbach I'm looking into this now.

We're also experiencing this. In our case, it happens if we put anything in the inactiveChild snippet here. Strangly, activeChild isn't an issue.

https://github.com/skeletonlabs/skeleton/blob/2a14a661cba9bf54018f94bcad6d5211a0ec8c98/packages/skeleton-svelte/src/lib/components/Switch/Switch.svelte#L116-L119

@trueadm
Copy link
Contributor Author

trueadm commented Dec 6, 2024

#14589 should fix this issue

@dummdidumm
Copy link
Member

Please open an issue with a self-contained REPL reproduction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Svelte 5: {#if} blocks are not tree shaken... shook?
7 participants