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

Force TypeScript only defineProps #1959

Closed
mesqueeb opened this issue Sep 5, 2022 · 7 comments · Fixed by #1968
Closed

Force TypeScript only defineProps #1959

mesqueeb opened this issue Sep 5, 2022 · 7 comments · Fixed by #1968

Comments

@mesqueeb
Copy link
Contributor

mesqueeb commented Sep 5, 2022

Please describe what the rule should do:
Forces the TypeScript only way of defining props.

What category should the rule belong to?

[x] Enforces code style (layout)
[ ] Warns about a potential error (problem)
[ ] Suggests an alternate way of doing something (suggestion)
[ ] Other (please specify:)

Provide 2-3 code examples that this rule should warn about:

GOOD

const props = defineProps<{
  /**
   * The kind of button to render
   */
  kind: 'primary' | 'secondary',
}>()

BAD

const props = defineProps({
  /**
   * The kind of button to render
   */
  kind: { type: String as PropType<'primary' | 'secondary'> },
})

BAD

const props = defineProps({
  /**
   * The kind of button to render
   */
  kind: { type: String },
})
@ota-meshi
Copy link
Member

Thank you for the rule suggestions! That rule sounds good to me.
Specifically, I think the rule should only check if lang="ts" is specified.

I would prefer to name the rule prefer-type-props-decl, what do you think?

@mesqueeb
Copy link
Contributor Author

mesqueeb commented Sep 5, 2022

@ota-meshi yeah, this sounds good to me!

I've read through some other threads and saw that you're not too experienced with the TS AST tree.
Is this something you're open to exploring now?

I'd be willing to put down a one time sponsorship for this specific issue if you want to challange this one. : )

@ota-meshi
Copy link
Member

I'm not familiar with TS AST (and TS syntax). If someone opens a PR, we'll merge it, but if there's a bug in the added rule, it's possible that I can't fix it. I need help from the community.

@Amorites
Copy link
Contributor

I would love to implement this rule.

Here are some thoughts, please help to review.

Basic process flow

  • If no lang="ts" specified, return directly
  • Get definePropsNodes. For each node, if node.arguments.length is greater than 0, report an error.

Questions

  • The defineEmits rule has similar behavior, should we implement it as a separate rule or as an option to this rule

Lower priority features (maybe in a separate PR next time)

  • autofix

@mesqueeb
Copy link
Contributor Author

This sounds great!

As for combining it with emit or not, I don't mind either way.

Personally I will always use both of them but I'm not sure if some ppl wanna split them up.

Split into two rules might make the documentation a bit easier to digest as well.

@ota-meshi
Copy link
Member

Thank you for your contribution.
We prefer to split the rules in two to keep the implementation of each rule simple. I think auto-fix implementations are probably very different.
I recommend linking it as a relevant rule in rule documentation.

@mesqueeb
Copy link
Contributor Author

@Amorites btw, I prepared the defineEmits one in this separate issue:
#1960

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants