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(vue): Component default scoped slot value types #451

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

wobsoriano
Copy link
Contributor

@wobsoriano wobsoriano commented Sep 8, 2023

Hi! This PR adds typings to the Subscribe and Field component default slot values.

Screenshot 2023-09-08 at 12 22 55 AM Screenshot 2023-09-08 at 12 17 50 PM

Some reference: vuejs/core#7982

Tests passed ✅

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 8, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@wobsoriano
Copy link
Contributor Author

wobsoriano commented Sep 8, 2023

We could make the Field component slot value type-safe as well but the way values are currently passed to the default slot makes it difficult to type:

Screenshot 2023-09-08 at 12 34 25 AM

If interested I can update it to pass an object instead (breaking) - in that way we can type it like this PR:

- return () => context.slots.default!(fieldApi.api, fieldApi.state.value)
+ return context.slots.default!({
+   field: fieldApi.api,
+   state: fieldApi.state.value
+ })
- context: SetupContext,
+ context: SetupContext<EmitsOptions, SlotsType<{ default: {
+    field: FieldApi<FieldValue<TParentData, TField>, TFormData>,
+    state: FieldState<any>
+  } }>

@crutchcorn
Copy link
Member

Now's the best time to do breaking changes, let's do it! 😄

Thanks for this change.

BTW, have you seen the typings for FieldComponent? They're pretty gross and type out children:

export type FieldComponent<TParentData, TFormData> = <TField>(
fieldOptions: {
children?: (
fieldApi: FieldApi<FieldValue<TParentData, TField>, TFormData>,
) => any
} & Omit<
UseFieldOptions<FieldValue<TParentData, TField>, TFormData>,
'name' | 'index'
> &
(TParentData extends any[]
? {
name?: TField extends undefined ? TField : DeepKeys<TParentData>
index: number
}
: {
name: TField extends undefined ? TField : DeepKeys<TParentData>
index?: never
}),
context: SetupContext,
) => any

I don't think this is right, but no matter what I tried to update, I couldn't get the TSX in our tests working properly and had to resort to manually typing field:

{(field: FieldApi<string, Person>) => <p>{field.state.value}</p>}

This would be amazing to get fixed, any chance you have an idea of how to do so?

@wobsoriano wobsoriano changed the title fix(vue): Subscribe component default scoped slot types fix(vue): Component default scoped slot value types Sep 8, 2023
@wobsoriano
Copy link
Contributor Author

wobsoriano commented Sep 8, 2023

Added an update for the Field component as well.

BTW, have you seen the typings for FieldComponent

@crutchcorn Unfortunately no, I think typed scoped slots only supports SFC as of now. Even tried to do a quick component with a default slot to check its type:

Screenshot 2023-09-08 at 11 35 22 AM

That's plain TS and it can't still pick up the slot value types.

@crutchcorn
Copy link
Member

That's plain TS and it can't still pick up the slot value types.

That's what I ran into as well..

Do we wanna just remove the children typing then as part of this Vue adapter type cleanup?

@wobsoriano
Copy link
Contributor Author

@crutchcorn Done!

Copy link
Member

@crutchcorn crutchcorn left a comment

Choose a reason for hiding this comment

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

Great work, thank you for your help and patience!

@crutchcorn crutchcorn merged commit 6935b33 into TanStack:main Sep 8, 2023
5 checks passed
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.

2 participants