-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
feat(tsx): add support for passing generics to child components #11478
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
Conversation
Oh nice job 👍 |
❌ Deploy Preview for vue-sfc-playground failed.
|
CI fails with updating branch. How can I fix it? |
Size ReportBundles
Usages
|
@@ -1704,5 +1706,104 @@ describe('api: options', () => { | |||
`Computed property "foo" is already defined in Data.`, | |||
).toHaveBeenWarned() | |||
}) | |||
|
|||
test('defineComponent with generic', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the tests are related to types should be placed in packages/dts-test/defineComponent.test-d.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edison1105
thank you!
I moved it to defineComponent.test-d.tsx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still not quite right, you need to refer to the exist test cases in the defineComponent.test-d.tsx
file to add new test cases. Check the type only, do not test the render result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edison1105
Modified! It's my insufficient understanding. Would you review the changes once more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those test cases seem not to be related generics. you should refer to this test case
describe('function syntax w/ generics', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edison1105
My understanding of the tests was completely lacking. What do you think of this revision?
I'd appreciate your review.
commit: @vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
|
extend({ name: options.name }, extraOptions, { setup: options }))() | ||
export function defineComponent(options: unknown): any { | ||
const comp = isFunction(options) | ||
? { setup: options, name: options.name } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this change? It doesn't seem to be related to types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edison1105
The original code had logic to convert function components into objects, but I changed the runtime behavior using Proxy. This was intended to handle props when using Generic Components in TSX.
As you pointed out, this isn't related to types. I realized that by removing extraOptions in this modification, I might have eliminated the ability to extend types.
I'm planning to revert these changes, but is my thought process correct?
Thank you for reviewing multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intended to handle props when using Generic Components in TSX.
-
I am not sure about this, keep this, waiting for other team members' review
-
It looks like the test case is incorrect because it still passes without the changes in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for review!
It looks like the test case is incorrect because it still passes without the changes in this PR.
Does the fact that the test passes without this change mean that the following writing style already exists?
core/packages-private/dts-test/defineComponent.test-d.tsx
Lines 2058 to 2061 in e46dfdd
// Test GenericComp directly with correct props | |
expectType<JSX.Element>( | |
<GenericComp<{ name: string }> msg="hello" list={[{ name: 'Alice' }]} />, | |
) |
Or is there a possibility that the original type checking was loose and designed not to throw errors regardless of the writing style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edison1105
It has been left here for a long time in a state where I can't proceed with anything. Is there anything I can do?
Issue
close #11374
Description
Based on the above issue, changed it to be able to use generics on defineComponent.
I'm not sure if this direction is right in the first place because a bigger change has been made than I thought.
This is my first time writing a PR here, so there can be a lot of mistakes. I want to continue contributing in the future, please advise me.