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: relax Component type #11929

Merged
merged 1 commit into from
Jun 6, 2024
Merged

fix: relax Component type #11929

merged 1 commit into from
Jun 6, 2024

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jun 6, 2024

The current type narrows the binding type to "" by default, which means "no bindings on this component". While this is the common case, it makes it very cumbersome to use the Component type because legacy components are of type string and as soon as you have bindings, the type is something like "foo" | "bar" which also is not assignable to "" which is semantically wrong, because you should be able to assign a component that can have bindings to a type that accepts none. The pragmatic solution is to change the binding type to be string by default, which means someone theoretically could use bindings with a component that doesn't have bindings:

<script>
  let component: Component<{ prop: boolean }> = IAcceptNoBindings;
</script>
<!-- allowed but should be a type error -->
<svelte:component this={component} bind:prop={foo} />

But this is a) rare anyway and b) can be caught at runtime

This came up in comments of #11775

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

The current type narrows the binding type to `""` by default, which means "no bindings on this component". While this is the common case, it makes it very cumbersome to use the `Component` type because legacy components are of type `string` and as soon as you have bindings, the type is something like `"foo" | "bar"` which _also_ is not assignable to `""` which is semantically wrong, because you should be able to assign a component that can have bindings to a type that accepts none.
The pragmatic solution is to change the binding type to allow `string`, which means someone theoretically could use bindings with a component that doesn't have bindings:
```svelte
<script>
  let component: Component<{ prop: boolean }> = IAcceptNoBindings;
</script>
<!-- allowed but should be a type error -->
<svelte:component this={component} bind:prop={foo} />
```
But this is a) rare anyway and b) can be caught at runtime

This came up in comments of #11775
Copy link

changeset-bot bot commented Jun 6, 2024

🦋 Changeset detected

Latest commit: 4f1f8d8

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 Rich-Harris merged commit 5b0a843 into main Jun 6, 2024
8 checks passed
@dummdidumm dummdidumm deleted the relax-component-type branch June 6, 2024 14:12
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

2 participants