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

feat: add possibility to decorate components #11544

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dummdidumm
Copy link
Member

#11472 brought up a gap we have with the new API currently: There's no way to decorate a component right now. With the old class syntax this was straightforward because you would extend the class and for example modify the props on the way in.

This is a proposal to add decorateComponent to achieve the same.
TODO:

  • agree on whether or not we want this
  • agree on name
  • add entry to docs
  • add tests

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

#11472 brought up a gap we have with the new API currently: There's no way to decorate a component right now. With the old class syntax this was straightforward because you would extend the class and for example modify the props on the way in. This adds `decorateComponent` to achieve the same.
Copy link

changeset-bot bot commented May 10, 2024

🦋 Changeset detected

Latest commit: 275fd2e

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

This does feel a bit YAGNI to me. I'd be much happier just exposing the correct types so that people are free to satisfy these advanced use cases. Obviously the first parameter (node or $$payload) would need to remain an implementation detail because of the client/server thing, so it could have a SvelteInternalsDoNotUseOrYouWillBeFired type, but beyond that it feels like obscuring things for the sake of obscuring things.

The fact is we don't keep these implementation details a secret — we display them proudly in the JS Output tab of the REPL, and reference them in talks about How Svelte Works etc. It feels weird to do that but then make people jump through hoops when the function signature is right there.

@dummdidumm
Copy link
Member Author

dummdidumm commented May 12, 2024

We agreed a while back to keep the component type as a class publicly because it makes typing scenarios easier. We would have to change that (and tooling etc) if we were to make this functions officially. I don't think that's worth it (we tried and then agreed it's too much work for no real gain).
Furthermore, yes it feels YAGNI now, but imagine we had this and mount etc from day one - we wouldn't have to tell people about these Svelte 5 breaking changes and how to migrate because the public API wouldn't have to change and we're free to switch from classes to functions under the hood. Similarly, in a distant future we might want to switch back for some reason, or change the call signature, at which point we're again faced with this problem.
Showing this in the component output tab isn't a good comparison IMO - that's like saying "yes it's functions, use it like that, oh and by the way go ahead and use svelte/internal because we use that in this output tab, too".

@Rich-Harris
Copy link
Member

I don't follow that logic. The API has changed, and the types should reflect that. Right now, this yields no type errors:

import Thing from './Thing.svelte';

class WrappedThing extends Thing {
  constructor(options) {
    super({
      ...options,
      props: { ...options.props, answer: 42 }
    })
  }
}

That's unacceptable. At runtime, it fails cryptically — with a "Class constructor WrappedThing cannot be invoked without 'new'" message in SSR, and a wall of text containing internal implementation details in CSR.

Similarly, in a distant future we might want to switch back for some reason, or change the call signature, at which point we're again faced with this problem.

We should optimise for the present day, not some hypothetical future that may or may not come to pass. Had we designed some equivalent API in the past, it's very likely that we'd have either a) failed to anticipate the switch from classes to functions, forcing a different breaking change, or b) got in the way of people doing hacky-but-clever things with class extensions.

@qwuide
Copy link

qwuide commented May 13, 2024

What's appealing to me about a function like this (if I understand it correctly?) is that it will enable very powerful component compositions in combination with snippets. Similar to contextual components that have been around for a long time in Ember.js.

@Rich-Harris
Copy link
Member

The closest thing to Ember's {{component ...}} is <svelte:component this={...} />

@qwuide
Copy link

qwuide commented May 14, 2024

The closest thing to Ember's {{component ...}} is <svelte:component this={...} />

That's true, but I was thinking more about the aspect where you can "curry" a component with some contextual props and then pass it around. With the decorateComponent function, it could look something like this in Svelte, using snippets:

+page.svelte

<script>
  import Form from '$lib/components/Form.svelte';

  let object = $state({ name: 'Joe' });
</script>

<Form bind:object>
  {#snippet children(form)}
    <form.Item key="name">
      {#snippet children(item)}
        <item.Label>
          Name
        </item.Label>
        <item.Input />
      {/snippet}
    </form.Item>
    <form.Submit>
      Save
    </form.Submit>
  {/snippet}
</Form>

Form.svelte

<script>
  import { decorateComponent } from 'svelte';

  import FormItem from './FormItem.svelte';
  import Button from './Button.svelte';

  const { object = $bindable(), children } = $props();

  function handleItemChange(key, value) {
    object[key] = value
  }
</script>

<form>
  {@render children({
    Item: decorateComponent(FormItem, {onchange: handleItemChange}),
    Submit: decorateComponent(Button, {type: 'submit'}),
  })}
</form>

FormItem.svelte

<script>
  import { decorateComponent } from 'svelte';

  import Label from './Label.svelte';
  import Input from './Input.svelte';

  const { key, children, onchange } = $props();

  function handleInputChange(value) {
    onchange(key, value)
  }

</script>

{@render children({
  Label: decorateComponent(Label, {labelFor: key}),
  Input: decorateComponent(Input, {
    id: key,
    name: key,
    onchange: handleInputChange,
  }),
})}

That said, I might have misunderstood the function's use case, but if it can be used in this way, it would open up for very powerful component composition patterns.

@Rich-Harris
Copy link
Member

Wouldn't it be preferable to use context for that?

<script>
  import { Form } from './form.js';
	
  let object = $state({ name: 'Joe' });
</script>

<Form bind:object>
  <Form.Item key="name">
    <Form.Label>Name</Form.Label>
    <Form.Input />
  </Form.Item>

  <Form.Submit>
    Save
  </Form.Submit>
</Form>

@qwuide
Copy link

qwuide commented May 15, 2024

Wouldn't it be preferable to use context for that?

<script>
  import { Form } from './form.js';
	
  let object = $state({ name: 'Joe' });
</script>

<Form bind:object>
  <Form.Item key="name">
    <Form.Label>Name</Form.Label>
    <Form.Input />
  </Form.Item>

  <Form.Submit>
    Save
  </Form.Submit>
</Form>

Nice! That's indeed another way to do it. But one advantage of "currying" the components instead of using context is that it will make it possible to combine it with generic types. For example, in the <Form> case, one could have the object prop as a generic type and then get type safety on the key prop for the <FormItem> component.

I haven't tried it, but I believe something along these lines should make that possible:

<script lang="ts" generics="T extends Record<string, unknown>">
  import type { ComponentProps, Snippet } from 'svelte';
  import { decorateComponent } from 'svelte';

  import Button from './Button.svelte';
  import FormItem from './FormItem.svelte';

  interface Props {
    object: T;
    children: Snippet<
      [
        {
          Item: Omit<ComponentProps<FormItem>, 'key'> & { key: keyof T };
          Submit: ComponentProps<Button>;
        }
      ]
    >;
  }

  let { object = $bindable(), children }: Props = $props();

  handleItemChange(key: string, value: unknown) {
    object = { ...object, [key]: value };
  }
</script>

<form>
  {@render children({
    Item: decorateComponent(FormItem, { onchange: handleItemChange }),
    Submit: decorateComponent(Button, { type: 'submit' })
  })}
</form>

Of course, one could still have achieved this without decorateComponent by instead combining context and yielded components, but in my opinion, decorateComponent would make this pattern a bit more straightforward by avoiding the need to couple generic components (like <Input>, <Select>, etc.) to context or the requirement to create wrapper components for them; instead, you can just use them as they are.

However, when comparing our examples and how the components are composed in the end, yours stands out as cleaner, but I believe that's mostly because yielding props to snippets requires a few extra lines of code.

As a side note and a bit off-topic, I would love a snippet syntax similar to this:

<Form bind:object (form)>
  <form.Item key="name" (item)>
    <item.Label>Name</item.Label>
    <item.Input />
  </form.Item>
  <form.Submit>Submit</form.Submit>
</Form>

Handling multiple snippets could look something like this (similar to named blocks in Ember.js):

<PopoverTrigger>
  <:trigger (props)>
    <Button {...props}>
      Open popover
    </Button>
  </:trigger>
  <:popover (props)>
    <Popover {...props}>
      Hello!
    </Popover>
  </:popover>
</PopoverTrigger>

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.

None yet

3 participants