implements autofix in define-props-declaration (#2465)#2466
implements autofix in define-props-declaration (#2465)#2466mpiniarski wants to merge 28 commits intovuejs:masterfrom
Conversation
…ased syntax (vuejs#2465) handle native types (String, Boolean etc.)
…ased syntax (vuejs#2465) handle PropTypes (e.g. String as PropType<'test'>)
…ased syntax (vuejs#2465) handle required option
…ased syntax (vuejs#2465) handle default option
…ased syntax (vuejs#2465) handle separateInterface rule option
…ased syntax (vuejs#2465) bring back the runtime check
…ased syntax (vuejs#2465) additional tests and refactoring
…ased syntax (vuejs#2465) documentation update
…ased syntax (vuejs#2465) fix tests failing on some environments
…ased syntax (vuejs#2465) handle array of types
…ased syntax (vuejs#2465) refactoring
…ased syntax (vuejs#2465) copy type for unknown expressions, ignore fixing cases when error is thrown
…ased syntax (vuejs#2465) handle union type
|
@FloEdelmann can I get some attention to this PR? |
FloEdelmann
left a comment
FloEdelmann
left a comment
There was a problem hiding this comment.
Thanks for the ping! I had an initial look now.
Co-authored-by: Flo Edelmann <git@flo-edelmann.de>
mpiniarski
force-pushed
the
feature/#2465_autofix_in_define-props-declaration
branch
from
28aab32 to
7d9e731
Compare
|
@FloEdelmann applied changes, please review again |
ota-meshi
left a comment
ota-meshi
left a comment
There was a problem hiding this comment.
Thank you for working on that implementation!
It seems to be causing an error in CI. Could you please fix that as well?
|
@ota-meshi Could you please clear the Netlify cache and restart the deployment? That would make the rule (and docs) changes easier to test. |
|
I cleared the cache and restart. The PR status remains failed, but it seems to have worked. |
FloEdelmann
left a comment
FloEdelmann
left a comment
There was a problem hiding this comment.
Some more small JSDoc suggestions, then this is good to go from my side! Thanks for the contribution!
mpiniarski
force-pushed
the
feature/#2465_autofix_in_define-props-declaration
branch
from
9cca135 to
e9d4400
Compare
|
Should we fix it to multi-line interface, then make the separator configurable? |
|
No, let's keep it single-line and leave the rest to other ESLint rules or formatters like Prettier. |
| */ | ||
| function getComponentPropData(prop, sourceCode) { | ||
| if (prop.propName === null) { | ||
| throw new Error('Unexpected prop with null name.') |
There was a problem hiding this comment.
Why does it need to throw an error here?
Can't we make it guard against those before processing the autofix?
There was a problem hiding this comment.
What do you mean?
I think it's more clear if we throw an error here.
There was a problem hiding this comment.
I think that if we throw an error, the user's eslint command will fail.
Did you throw the error with that intention? If so, why is that?
If auto-fix is not possible, I think it should be handled so that auto-fix is not performed, rather than throwing an error.
There was a problem hiding this comment.
I see. Let me catch errors and ignore them.
I prefer to leave a sign of an error somewhere, maybe in console.debug(), but it is up to you.
| filename: 'test.vue', | ||
| code: ` | ||
| <script setup lang="ts"> | ||
| const props = defineProps([kind]) |
There was a problem hiding this comment.
The array elements must be strings. The kind variable shouldn't auto-fix because we don't know what's actually in it.
Could you leave this test as a test case for when not to auto-fix and add a new test case using a string literal?
https://vuejs.org/guide/components/props.html#props-declaration
There was a problem hiding this comment.
My bad, you're right. It does not default to a string. Leaving it unhandled then.
There was a problem hiding this comment.
Could you please add the test case you requested earlier?
<script setup lang="ts">
const props = defineProps(['kind'])
</script>
|
@ota-meshi can you take a look? |
| */ | ||
| function getComponentPropData(prop, sourceCode) { | ||
| if (prop.type === 'array') { | ||
| throw new Error(`Unable to resolve types based on array prop declaration.`) |
There was a problem hiding this comment.
Could you please implement it so that the autofix stops without throwing an error?
4 participants
Fixes #2465