feat: Variadic snippets #9988

elliott-with-the-longest-name-on-github · 2023-12-23T04:04:48Z

A REPL showing that this handles every case I could think of

Summary

This would close #9672, assuming we want it. I kind of like it!

This allows snippets to have multiple arguments in the type system. For example, you could type a component expecting a two-argument snippet as:

<script lang="ts">
  let {
    message,
    card
  } = $props<{
    message: string;
    card: Snippet<[message: string, darkMode: boolean]>;
  }>(); 
</script>

{@render card(message, false)}

Since spreading is also now acceptable, you could also:

{@render snippet(...['as', 'many', 'strings', 'as', 'i', 'want'])}

The only gotcha being that spreading a $state array would lose reactivity unless the items are boxed state.

Implementation notes:

Adding additional arguments was easy -- basically just improving the parsing for snippet blocks, removing a "one arg only" check in one place, and sticking loops in some other places.

Adding support for rest params and spread arguments was harder. In the case of spread arguments, a function call like this:

foo(...bar)

is a problem for us, because when compiling calls to snippet functions, we turn each argument into a thunk. We obviously can't thunk bar in this case:

foo(...() => bar)

because it would explode. The only solution I could come up with was to basically deferred-thunk rest arguments, so:

// this...
foo(1, 2, ...rest)

// ...becomes this
foo(() => 1, () => 2, ...$.thunkspread(rest))

// ...where `thunkspread` is just:
export function thunkspread(iterable) {
  const thunks = [];
  for (const item of iterable) {
  thunks.push(() => item);
  }
  // @ts-expect-error -- for obvious reasons
  return thunks;
}

I don't think this is any significant performance penalty; JavaScript would already have to turn any spread iterable into an array anyway, so basically all we're doing is the additional runtime thunkification. It'd be worth testing, though, to make sure we're not significantly regressing.

In the case of rest params, the problem was even more complicated. A "normal" snippet gets compiled like this:

{#snippet foo(one, { two })}
  <div>{one}, {two}</div>
{/snippet}

becomes:

function foo($$anchor, one, $$arg1) {
  let two = () => $$arg1().two;
  /* Init */
  var div = $.open($$anchor, true, frag_1);
  var text = $.child(div);
  
  /* Update */
  $.text_effect(text, () => `${$.stringify(one())}, ${$.stringify(two())}`);
  $.close($$anchor, div);
}

Basically, we make sure that all of the arguments to the function are accessible by their original names -- for non-destructured arguments, we do nothing; for destructured arguments, we find all of the destructured identifiers and thunkify them into variables that can then be called elsewhere (as you can see with one() and two().

With rest params, we lose the ability to do this -- because ...rest is an array, we can't anticipate when or where the actual items inside will be accessed, and therefore can't make sure their thunkified values are called. rest[0] would result in an uncalled thunk rather than the value expected!

The only solution I could come up with for this was to wrap the rest array in a super simple proxy:

export function proxy_rest_array(items) {
  return new Proxy(items, {
    get(target, property) {
      if (typeof property === 'symbol') return target[property];
      if (!isNaN(parseInt(property))) {
      return target[property]();
      }
      return target[property];
    }
  });
}

This basically says "if you try to access a numeric property on this array, I'll return the value behind the function at that index" -- it's the exact opposite of thunkspread. This also works with other array methods like find and map, which have to use numeric property accessors to get their values out of the array.

This enables the compiler to do this:

<script>
  let two = $state(2);
</script>
<button onclick={() => two++}>Update</button>
{#snippet test(one, two, three, {four, five}, ...six)}
  {@const testElement = six.find((_, i) => i === 0)}
  <div>{four}, {five}, {two}, {three}, {one}, {six[0]}</div>
{/snippet}
{@render test(...[1, two, 3, {four: 4, five: 5}])}

becomes this:

  function test(
    $$anchor,
    one,
    two,
    three,
    $$arg3,
    ...$$arg4
  ) {
    let four = () => $$arg3().four;
    let five = () => $$arg3().five;
    let six = $.proxy_rest_array($$arg4);
    /* Init */
    var div = $.open($$anchor, true, frag_1);
    const testElement = $.derived(() => six.find((_, i) => i === 0));
    var text = $.child(div);
    
    /* Update */
    $.text_effect(text, () => `${$.stringify(four())}, ${$.stringify(five())}, ${$.stringify(two())}, ${$.stringify(three())}, ${$.stringify(one())}, ${$.stringify(six[0])}`);
    $.close($$anchor, div);
  }

Is there a performance penalty? Yes, but crucially, only for rest params -- other params are unaffected. And in practice, I'm not sure how big that penalty actually is over the penalty faced by normal rest params.

Before submitting the PR, please make sure you do the following

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
Prefix your PR title with feat:, fix:, chore:, or docs:.
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests and linting

Run the tests with pnpm test and lint the project with pnpm lint

changeset-bot · 2023-12-23T04:04:53Z

🦋 Changeset detected

Latest commit: 84c6eda

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

vercel · 2023-12-23T04:07:54Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
svelte-5-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 15, 2024 11:30pm

Rich-Harris · 2024-01-31T00:19:57Z

@tcc-sejohnson at one stage we had planned to make the [...] unnecessary in the common case of a single argument. I gather from #9672 that we abandoned that because of rest arguments, but given that we ended up not supporting them, does that still hold? Or are you still hoping that we can somehow make them work in future?

elliott-with-the-longest-name-on-github · 2024-01-31T00:22:02Z

@tcc-sejohnson at one stage we had planned to make the [...] unnecessary in the common case of a single argument. I gather from #9672 that we abandoned that because of rest arguments, but given that we ended up not supporting them, does that still hold? Or are you still hoping that we can somehow make them work in future?

I think it's just... slightly uncanny the other way, and shrug no reason to prevent ourselves from doing rest args in the future if we want to.

packages/svelte/src/main/private.d.ts

sites/svelte-5-preview/src/routes/docs/content/01-api/03-snippets.md

Rich-Harris

almost done with review, just taking a break for dinner

Rich-Harris · 2024-01-31T02:02:59Z

struggling a bit with the CI failure. can't reproduce locally. that's the only thing blocking merging right now

Rich-Harris · 2024-01-31T02:06:28Z

actually scratch that, I can reproduce it. haven't figured it out yet though it does appear to be related to recent changes to {@const ...} parsing

…in (???)

The generated types were invalid TypeScript ("cannot use unique symbol at this position"). Use utility types to ensure the types are not unpacked during type generation. Leftover from #9988

@const

* give this another try * fix: lint * fix: Forgot to save * feat: it works boiiii * look, ok, it did work, i just needed to update the snapshots * bruh * changeset * feat: ok I think the client snippet block finally works * feat: current tests pass; I'm sure I'm missing stuff for new things * fix: snapshot * feat: I think non-destructured rest should work now? * chore: duplicated computation * feat: Tests (passing and failing * feat: it's... alive? * chore: Clean up my messes * chore: devtime stuff * fix: fmt * chore: see if this fixes repl * chore: make naming more offensive * fix: Don't throw on missing keys, return undefined as it usually would * Update packages/svelte/src/compiler/phases/1-parse/state/tag.js Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> * Update packages/svelte/src/compiler/phases/1-parse/state/tag.js Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> * fix: Hopefully default param values now work * dumb * types * feat: Test it * fix: Turns out javascript parameters are optional * feat: The Final Solution * document function * feat: Better bracket matching, unit tests * feat: exclude test files from publish * feat: More unit tests * feat: Use more efficient parsing for @const * Update .changeset/curvy-cups-cough.md Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> * Update packages/svelte/package.json Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> * Update packages/svelte/src/compiler/phases/1-parse/utils/bracket.js Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> * fix: changesets * chore: additional comments * fix: kill foreach * fix: foreach again * feat: Docs * Revert "fix: kill foreach" This reverts commit 9a688cc. * fix: My own stupidity * fix: style * fix - maybe * Update sites/svelte-5-preview/src/routes/docs/content/01-api/03-snippets.md * Update tag.js Co-authored-by: Rich Harris <richard.a.harris@gmail.com> * Update .changeset/curvy-cups-cough.md Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> * chore: Remove rest params * Delete .changeset/eighty-rivers-wash.md * fix: Honestly idk why it was broken but it's fixed now * fix: var name lol * fix: typegen * fix: idk * fix: It looks like a bunch of unformatted shit came in through main?? idk * Revert "fix: It looks like a bunch of unformatted shit came in through main?? idk" This reverts commit ab851d5. * fix: format again * this is getting ridiculous * Update tag.js Co-authored-by: Rich Harris <richard.a.harris@gmail.com> * fix errors * simplify a bit * use read_context * use read_context for const as well * remove unused code * unused import * unused export * remove spread args. sorry elliott * tidy up SnippetBlock interface * fix test * simplify * tweak * revert example, so that it matches the surrounding text * move PropsWithChildren back to public.d.ts * update typing docs, so that it flows from previous example * temporarily revert const parsing changes, to get prettier working again (???) * oops --------- Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> Co-authored-by: Rich Harris <richard.a.harris@gmail.com> Co-authored-by: Rich Harris <rich.harris@vercel.com>

The generated types were invalid TypeScript ("cannot use unique symbol at this position"). Use utility types to ensure the types are not unpacked during type generation. Leftover from #9988

elliott-with-the-longest-name-on-github added 3 commits December 22, 2023 22:49

give this another try

7690371

fix: lint

14805f9

fix: Forgot to save

3756ce5

vercel bot deployed to Preview – svelte-5-preview December 23, 2023 04:07 View deployment

feat: it works boiiii

243ba9c

vercel bot deployed to Preview – svelte-5-preview December 23, 2023 04:32 View deployment

vercel bot deployed to Preview – svelte-dev December 23, 2023 04:32 View deployment

look, ok, it did work, i just needed to update the snapshots

cb4daea

vercel bot deployed to Preview – svelte-5-preview December 23, 2023 04:38 View deployment

vercel bot deployed to Preview – svelte-dev December 23, 2023 04:38 View deployment

bruh

8382eb0

vercel bot deployed to Preview – svelte-dev December 23, 2023 04:44 View deployment

vercel bot deployed to Preview – svelte-5-preview December 23, 2023 04:47 View deployment

elliott-with-the-longest-name-on-github changed the title ~~Elliott/variadic snippets~~ feat: Variadic snippets Dec 23, 2023

changeset

5bab2db

vercel bot deployed to Preview – svelte-5-preview December 23, 2023 04:58 View deployment

vercel bot deployed to Preview – svelte-dev December 23, 2023 04:58 View deployment

elliott-with-the-longest-name-on-github marked this pull request as ready for review December 23, 2023 05:00

elliott-with-the-longest-name-on-github added 2 commits December 24, 2023 01:08

feat: ok I think the client snippet block finally works

5beaf36

feat: current tests pass; I'm sure I'm missing stuff for new things

33a39bf

vercel bot deployed to Preview – svelte-dev December 24, 2023 06:50 View deployment

fix: snapshot

c1bb239

vercel bot deployed to Preview – svelte-dev December 24, 2023 06:53 View deployment

vercel bot deployed to Preview – svelte-5-preview December 24, 2023 06:57 View deployment

elliott-with-the-longest-name-on-github added 3 commits December 25, 2023 22:35

feat: I think non-destructured rest should work now?

3a726a9

chore: duplicated computation

8555681

feat: Tests (passing and failing

6d364aa

vercel bot deployed to Preview – svelte-dev December 26, 2023 05:06 View deployment

Rich-Harris reviewed Jan 31, 2024

View reviewed changes

packages/svelte/src/main/private.d.ts Outdated Show resolved Hide resolved

Rich-Harris reviewed Jan 31, 2024

View reviewed changes

sites/svelte-5-preview/src/routes/docs/content/01-api/03-snippets.md Outdated Show resolved Hide resolved

Rich-Harris requested changes Jan 31, 2024

View reviewed changes

Rich-Harris added 2 commits January 30, 2024 20:38

revert example, so that it matches the surrounding text

d258b82

move PropsWithChildren back to public.d.ts

3198e20

vercel bot deployed to Preview – svelte-5-preview January 31, 2024 01:51 View deployment

vercel bot deployed to Preview – svelte-dev January 31, 2024 01:51 View deployment

update typing docs, so that it flows from previous example

0107dc3

vercel bot deployed to Preview – svelte-dev January 31, 2024 01:59 View deployment

vercel bot deployed to Preview – svelte-5-preview January 31, 2024 02:02 View deployment

temporarily revert const parsing changes, to get prettier working aga…

069ad1d

…in (???)

vercel bot deployed to Preview – svelte-dev January 31, 2024 02:16 View deployment

Rich-Harris approved these changes Jan 31, 2024

View reviewed changes

vercel bot deployed to Preview – svelte-5-preview January 31, 2024 02:19 View deployment

oops

84c6eda

vercel bot deployed to Preview – svelte-dev January 31, 2024 02:20 View deployment

Rich-Harris merged commit d309a9d into main Jan 31, 2024

vercel bot deployed to Preview – svelte-5-preview January 31, 2024 02:23 View deployment

github-actions bot mentioned this pull request Jan 31, 2024

Version Packages (next) #10327

Merged

Rich-Harris mentioned this pull request Jan 31, 2024

chore: simpler const parsing #10347

Merged

5 tasks

dummdidumm deleted the elliott/variadic-snippets branch January 31, 2024 11:02

trueadm pushed a commit that referenced this pull request Jan 31, 2024

fix: adjust broken types

ae93e05

The generated types were invalid TypeScript ("cannot use unique symbol at this position"). Use utility types to ensure the types are not unpacked during type generation. Leftover from #9988

leandrohsilveira mentioned this pull request Feb 6, 2024

Types for snippets broken in @5.0.0-next.42 sveltejs/language-tools#2281

Closed

Rich-Harris mentioned this pull request Apr 15, 2024

#snippet rest args and @render spread? #11178

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Variadic snippets #9988

feat: Variadic snippets #9988

elliott-with-the-longest-name-on-github commented Dec 23, 2023 •

edited

Loading

changeset-bot bot commented Dec 23, 2023 •

edited

Loading

vercel bot commented Dec 23, 2023 •

edited

Loading

Rich-Harris commented Jan 31, 2024

elliott-with-the-longest-name-on-github commented Jan 31, 2024

Rich-Harris left a comment

Rich-Harris commented Jan 31, 2024

Rich-Harris commented Jan 31, 2024

feat: Variadic snippets #9988

feat: Variadic snippets #9988

Conversation

elliott-with-the-longest-name-on-github commented Dec 23, 2023 • edited Loading

Summary

Implementation notes:

Before submitting the PR, please make sure you do the following

Tests and linting

changeset-bot bot commented Dec 23, 2023 • edited Loading

🦋 Changeset detected

vercel bot commented Dec 23, 2023 • edited Loading

Rich-Harris commented Jan 31, 2024

elliott-with-the-longest-name-on-github commented Jan 31, 2024

Rich-Harris left a comment

Choose a reason for hiding this comment

Rich-Harris commented Jan 31, 2024

Rich-Harris commented Jan 31, 2024

elliott-with-the-longest-name-on-github commented Dec 23, 2023 •

edited

Loading

changeset-bot bot commented Dec 23, 2023 •

edited

Loading

vercel bot commented Dec 23, 2023 •

edited

Loading