vuejs/eslint-plugin-vuePublic

NotificationsYou must be signed in to change notification settings
Fork688
Star4.5k

implements autofix in define-props-declaration (#2465)#2466

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

mpiniarski wants to merge28 commits intovuejs:master

base:master

Choose a base branch

frommpiniarski:feature/#2465_autofix_in_define-props-declaration

Open

implements autofix in define-props-declaration (#2465)#2466

mpiniarski wants to merge28 commits intovuejs:masterfrommpiniarski:feature/#2465_autofix_in_define-props-declaration

Conversation

Copy link

mpiniarski commentedMay 27, 2024•
edited by FloEdelmann
Loading

Fixes#2465

mpiniarski added8 commits

May 27, 2024 14:58

feat: autofix indefine-props-declaration: runtime syntax to type-b…

4844612

…ased syntax (vuejs#2465)handle native types (String, Boolean etc.)

feat: autofix indefine-props-declaration: runtime syntax to type-b…

b770232

…ased syntax (vuejs#2465)handle PropTypes (e.g. String as PropType<'test'>)

feat: autofix indefine-props-declaration: runtime syntax to type-b…

99e2f3e

…ased syntax (vuejs#2465)handle required option

feat: autofix indefine-props-declaration: runtime syntax to type-b…

f0294e8

…ased syntax (vuejs#2465)handle default option

feat: autofix indefine-props-declaration: runtime syntax to type-b…

5a4a15e

…ased syntax (vuejs#2465)handle separateInterface rule option

feat: autofix indefine-props-declaration: runtime syntax to type-b…

583c0db

…ased syntax (vuejs#2465)bring back the runtime check

feat: autofix indefine-props-declaration: runtime syntax to type-b…

4499597

…ased syntax (vuejs#2465)additional tests and refactoring

feat: autofix indefine-props-declaration: runtime syntax to type-b…

17ac982

…ased syntax (vuejs#2465)documentation update

mpiniarski mentioned this pull request

May 27, 2024

Add autofix todefine-props-declaration: transform runtime syntax to type-based syntax#2465

Open

mpiniarski added5 commits

May 27, 2024 17:58

feat: autofix indefine-props-declaration: runtime syntax to type-b…

5e1d3b1

…ased syntax (vuejs#2465)fix tests failing on some environments

feat: autofix indefine-props-declaration: runtime syntax to type-b…

bc506f9

…ased syntax (vuejs#2465)handle array of types

feat: autofix indefine-props-declaration: runtime syntax to type-b…

f301546

…ased syntax (vuejs#2465)refactoring

feat: autofix indefine-props-declaration: runtime syntax to type-b…

207477e

…ased syntax (vuejs#2465)copy type for unknown expressions, ignore fixing cases when error is thrown

feat: autofix indefine-props-declaration: runtime syntax to type-b…

d08bed1

…ased syntax (vuejs#2465)handle union type

Copy link

Author

mpiniarski commentedJun 17, 2024

@FloEdelmann can I get some attention to this PR?

FloEdelmann requested changes

Jun 19, 2024

View reviewed changes

Copy link

Member

FloEdelmann left a comment

Choose a reason for hiding this comment

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

Thanks for the ping! I had an initial look now.

lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

tests/lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

lib/rules/define-props-declaration.jsShow resolvedHide resolved

lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

mpiniarskiand others added5 commits

June 21, 2024 14:16

Update lib/rules/define-props-declaration.js

6cb7153

Co-authored-by: Flo Edelmann <git@flo-edelmann.de>

fix: required default value = false

f6c205f

feature: rename autoFixToSeparateInterface option and describe it in …

536c6a1

…the docs

chore: extract fixTypeBased function

100065d

chore: refactor fixTypeBased function

7d9e731

mpiniarski force-pushed thefeature/#2465_autofix_in_define-props-declaration branch from28aab32 to7d9e731Compare

June 21, 2024 13:07

Copy link

Author

mpiniarski commentedJun 21, 2024

@FloEdelmann applied changes, please review again

chore: refactor componentPropsTypeCode creation

0715943

ota-meshi requested changes

Jun 30, 2024

View reviewed changes

Copy link

Member

ota-meshi left a comment

Choose a reason for hiding this comment

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

Thank you for working on that implementation!

It seems to be causing an error in CI. Could you please fix that as well?

docs/rules/define-props-declaration.md OutdatedShow resolvedHide resolved

lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

mpiniarski added3 commits

July 1, 2024 17:51

fix: fix tests failing

bbdc134

feature: remove autoFixToSeparateInterface option

2a1c654

Merge branch 'master' into feature/#2465_autofix_in_define-props-decl…

7cdf3ff

…aration

FloEdelmann reviewed

Jul 2, 2024

View reviewed changes

tests/lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

Fix tests

0e6ea56

FloEdelmann reviewed

Jul 2, 2024

View reviewed changes

lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

FloEdelmann reviewed

Jul 2, 2024

View reviewed changes

lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

Copy link

Member

FloEdelmann commentedJul 2, 2024

@ota-meshi Could you please clear the Netlify cache and restart the deployment? That would make the rule (and docs) changes easier to test.

Copy link

Member

ota-meshi commentedJul 2, 2024

I cleared the cache and restart. The PR status remains failed, but it seems to have worked.
https://deploy-preview-2466--eslint-plugin-vue.netlify.app/rules/define-props-declaration.html

FloEdelmann approved these changes

Jul 4, 2024

View reviewed changes

Copy link

Member

FloEdelmann left a comment

Choose a reason for hiding this comment

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

Some more small JSDoc suggestions, then this is good to go from my side! Thanks for the contribution!

lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

FloEdelmann requested a review fromota-meshi

July 4, 2024 07:19

feature: code cleanup

e9d4400

mpiniarski force-pushed thefeature/#2465_autofix_in_define-props-declaration branch from9cca135 toe9d4400Compare

July 4, 2024 07:38

FloEdelmann reviewed

Jul 4, 2024

View reviewed changes

lib/rules/define-props-declaration.js OutdatedShow resolvedHide resolved

Lint

0bd915b

Copy link

Member

so1ve commentedJul 9, 2024

Should we fix it to multi-line interface, then make the separator configurable?

Copy link

Member

FloEdelmann commentedJul 9, 2024

No, let's keep it single-line and leave the rest to other ESLint rules or formatters like Prettier.

ota-meshi requested changes

Jul 13, 2024

View reviewed changes

tests/lib/rules/define-props-declaration.jsShow resolvedHide resolved

lib/rules/define-props-declaration.js Outdated

		*/
		function getComponentPropData(prop, sourceCode) {
		if (prop.propName === null) {
		throw new Error('Unexpected prop with null name.')

Copy link

Member

Choose a reason for hiding this comment

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

Why does it need to throw an error here?
Can't we make it guard against those before processing the autofix?

Copy link

Author

mpiniarskiJul 15, 2024•
edited
Loading

Choose a reason for hiding this comment

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

What do you mean?
I think it's more clear if we throw an error here.

Copy link

Member

Choose a reason for hiding this comment

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

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.

Copy link

Author

mpiniarskiJul 26, 2024

Choose a reason for hiding this comment

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

I see. Let me catch errors and ignore them.

I prefer to leave a sign of an error somewhere, maybe inconsole.debug(), but it is up to you.

feature: handle array as props list (vuejs#2465)

1d58a2b

ota-meshi requested changes

Jul 16, 2024

View reviewed changes

tests/lib/rules/define-props-declaration.js

		filename: 'test.vue',
		code: `
		<script setup lang="ts">
		const props = defineProps([kind])

Copy link

Member

ota-meshiJul 16, 2024

Choose a reason for hiding this comment

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

The array elements must be strings. Thekind 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

Copy link

Author

mpiniarskiJul 26, 2024

Choose a reason for hiding this comment

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

My bad, you're right. It does not default to astring. Leaving it unhandled then.

Copy link

Member

ota-meshiSep 2, 2024

Choose a reason for hiding this comment

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

Could you please add the test case you requested earlier?

      <script setup lang="ts">const props=defineProps(['kind'])</script>

#2466 (comment)

mpiniarski added2 commits

July 26, 2024 21:45

feature: catch errors and ignore them (vuejs#2465)

da17d74

feature: do not handle array prop declaration (vuejs#2465)

6634c2d

Copy link

Author

mpiniarski commentedJul 30, 2024

@ota-meshi can you take a look?

ota-meshi requested changes

Sep 2, 2024

View reviewed changes

lib/rules/define-props-declaration.js

		*/
		function getComponentPropData(prop, sourceCode) {
		if (prop.type === 'array') {
		throw new Error(`Unable to resolve types based on array prop declaration.`)

Copy link

Member

ota-meshiSep 2, 2024

Choose a reason for hiding this comment

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

Could you please implement it so that the autofix stops without throwing an error?

Labels

None yet

4 participants

Movatterモバイル変換

Uh oh!

implements autofix in define-props-declaration (#2465)#2466

Are you sure you want to change the base?

implements autofix in define-props-declaration (#2465)#2466

Uh oh!

Conversation

mpiniarski commentedMay 27, 2024• edited by FloEdelmannLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mpiniarski commentedJun 17, 2024

Uh oh!

FloEdelmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpiniarski commentedJun 21, 2024

Uh oh!

ota-meshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FloEdelmann commentedJul 2, 2024

Uh oh!

ota-meshi commentedJul 2, 2024

Uh oh!

FloEdelmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

so1ve commentedJul 9, 2024

Uh oh!

FloEdelmann commentedJul 9, 2024

Uh oh!

Uh oh!

ota-meshiJul 13, 2024

Choose a reason for hiding this comment

Uh oh!

mpiniarskiJul 15, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ota-meshiJul 16, 2024

Choose a reason for hiding this comment

Uh oh!

mpiniarskiJul 26, 2024

Choose a reason for hiding this comment

Uh oh!

ota-meshiJul 16, 2024

Choose a reason for hiding this comment

Uh oh!

mpiniarskiJul 26, 2024

Choose a reason for hiding this comment

Uh oh!

ota-meshiSep 2, 2024

Choose a reason for hiding this comment

Uh oh!

mpiniarski commentedJul 30, 2024

Uh oh!

ota-meshiSep 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpiniarski commentedMay 27, 2024•
edited by FloEdelmann
Loading

mpiniarskiJul 15, 2024•
edited
Loading