-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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: remove runtime validation of components/snippets, rely on types instead #12507
Conversation
🦋 Changeset detectedLatest commit: c27d8de The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
This doesn't yet update the implementation of server-side snippets to match the types, so we're not yet able to create snippets that curry arguments etc. That can be a follow-up, I reckon |
Not adding that feels incomplete - if we make the underlying types more visible, but it's still not actually the real underlying types, then we haven't won anything. |
Gah I thought you'd say that 😅 May not get as far as that this evening, we'll see |
Is it possible to split the validation rework out into its own PR? That work seems fairly uncontroversial, while the snippet type thing might take a few more discussions possibly. (Also I'm wondering if it makes sense / is possible to turn snippet arguments into an array under the hood, which also might open up the thing towards spreading) |
good idea — #12521 |
Per discussion, removed the controversial snippet changes, and restricted this PR to slightly tweaking the |
#12452 (comment).
Draft because the
render_tag_invalid_argument
message is now incorrect, but fixing it requires untangling a few other things. It turns out this particular validation is very broken at present:If you have a component like this...
...then
$$slots.default
is generated, and if you do{@render children()}
on the other side, it'll just be blank. Feels like at the very least it should error becausechildren
isundefined
, as we would for{@render nonexistent()}
if you do
{@render children('bob')}
then suddenly it errors. that makes no sense!if you do
let { children: x } = $props()
and{@render x('bob')}
it stops erroring again, because we're only looking for an identifier whose name ischildren
. This is brittle as all hellas a corollary: you can't declare a
{#snippet children(arg)}
inside the child component, because you'll get a false positive errorIn short: we need to rip the whole thing out. Seems like we'd be better off creating a dummy
children
prop in the parent that errors on behalf of the child when you try to render it.Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint