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(compiler-sfc): support defineAttrs macro #9413

Closed
wants to merge 1 commit into from

Conversation

rudyxu1102
Copy link
Contributor

@rudyxu1102 rudyxu1102 commented Oct 16, 2023

@github-actions
Copy link

github-actions bot commented Oct 16, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB (+38 B) 32.7 kB (+4 B) 29.5 kB (+6 B)
vue.global.prod.js 132 kB (+38 B) 49.3 kB (+5 B) 44.3 kB (-38 B)

Usages

Name Size Gzip Brotli
createApp 47.9 kB 18.8 kB 17.2 kB
createSSRApp 50.6 kB 19.9 kB 18.2 kB
defineCustomElement 50.3 kB 19.6 kB 17.9 kB
overall 61.2 kB 23.7 kB 21.6 kB

@rudyxu1102 rudyxu1102 changed the title feat: support defineAttrs macro feat(compiler-sfc): support defineAttrs macro Oct 16, 2023
@sadeghbarati sadeghbarati mentioned this pull request Oct 16, 2023
@sxzz
Copy link
Member

sxzz commented Oct 17, 2023

Is there any difference between this PR and #7444?

@sadeghbarati
Copy link

Is there any difference between this PR and #7444?

If I understand correctly, this PR is for typed/inferring attrs with macro in <script setup> (SFC)

and this PR #7444 is the same but for defineComponent or functional components (TS/TSX)

@sxzz
Copy link
Member

sxzz commented Oct 17, 2023

#7444 did the same thing (defineAttrs macro) before my review. As my review comment in #7444, I don't think it's necessary to introduce a new macro for attrs. Reusing useAttrs is enough for declaring types.

In you PR, defineAttrs will be replaced with useAttrs. So why not use it directly, without any transformation?

const attrs = defineAttrs<{
	bar?: number
}>()

Let's discuss in #7444 since they're basically doing the same feature.

@rudyxu1102
Copy link
Contributor Author

rudyxu1102 commented Oct 17, 2023

#7444 did the same thing (defineAttrs macro) before my review. As my review comment in #7444, I don't think it's necessary to introduce a new macro for attrs. Reusing useAttrs is enough for declaring types.

In you PR, defineAttrs will be replaced with useAttrs. So why not use it directly, without any transformation?

const attrs = defineAttrs<{
	bar?: number
}>()

Let's discuss in #7444 since they're basically doing the same feature.

I also planned to use useAttrs<T> at the beginning, but after I saw the implementation of defineSlots, I realize using useAttrs<T> without using <script setup> would confuse users.

e.g. Vue SFC Playground

Judging from the discussion in RFC#477, most people like defineAttrs macro and don't like the attrs option. I think it may be easier to discuss if it is separated into two PRs.

@sxzz sxzz changed the base branch from main to minor October 17, 2023 12:50
@sxzz
Copy link
Member

sxzz commented Oct 17, 2023

Sorry, I was misled and thought these two PRs belonged to two different authors. (P.S hope to have a more detailed PR description in the future :P )

Compared to defineSlots, you're right. However, we need only one generic support, which means maybe adding a macro defineAttrs is enough. For non <script setup>, users can reach attrs by args of setup function, which already has the right type declaration.

defineComponent({
  setup(_, { attrs }){}
})

Sorry again! I should discuss this with you before giving review suggestions. Please hold on, I will discuss it with Evan for a final decision.

@sxzz
Copy link
Member

sxzz commented Oct 17, 2023

After discussion, we decided to do something similar to defineSlots.
useAttrs will not be changed, but add a defineAttrs<T>() macro.

@sxzz
Copy link
Member

sxzz commented Oct 17, 2023

I think this PR and #7444 can be combined into one. WDYT?

@rudyxu1102
Copy link
Contributor Author

rudyxu1102 commented Oct 17, 2023

I think this PR and #7444 can be combined into one. WDYT?

Of course.Thank you for your positive response.

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.

3 participants