Skip to content

Comments

feat(nextcloud-vue): add rule for deprecated NcButton props#1045

Merged
susnux merged 1 commit intomainfrom
feat/nc-button-deprecations
May 28, 2025
Merged

feat(nextcloud-vue): add rule for deprecated NcButton props#1045
susnux merged 1 commit intomainfrom
feat/nc-button-deprecations

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented May 26, 2025

This is otherwise really a mess to migrate 🙈

@susnux susnux requested review from Antreesy, ShGKme and skjnldsv May 26, 2025 19:04
@susnux susnux added enhancement New feature or request 3. to review labels May 26, 2025
@susnux susnux force-pushed the feat/nc-button-deprecations branch from 96a17c5 to 275c533 Compare May 26, 2025 19:09
@Antreesy
Copy link
Contributor

Is it possible to check the following? I didn't get from the code:

:type="condition ? 'tertiary' : 'error'"
:type="buttonType"

const buttonType = computed(() => condition ? 'tertiary' : 'error')

version: packageVersion,
},
} satisfies ESLint.Plugin
} as unknown as ESLint.Plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to mismatch between ESLint type of rules and the typescript wrapper

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fix mismatch instead?

@Antreesy

This comment was marked as resolved.

@susnux
Copy link
Contributor Author

susnux commented May 27, 2025

@Antreesy this :type="\'secondary\'" should already be catched by other rules, you should never have useless v-bind like this. Should already catched by vue/no-useless-v-bind

@susnux
Copy link
Contributor Author

susnux commented May 27, 2025

Also what I did not implement to keep it simple: Aliasing the component.
Sometimes (for what reason ever) people alias the component like:

<template
<Button ...>
</template>
<script setup>
import Button from '@nextcloud/vue/components/NcButton'
</script>

Not sure if this is useful

@Antreesy
Copy link
Contributor

Aliasing the component

need to check ImportDeclaration as well then =), but then there goes a ton of fun with cases, like import { NcButton as Button } from '@nextcloud/vue'. Not sure it's worth it.

I'd be happy to cover 'ConditionalExpression' at least, then it's not much left to check manually

@ShGKme
Copy link
Contributor

ShGKme commented May 27, 2025

for what reason ever

It's from the time when there was no Nc* prefix yet, and then migrated by updating the import without changing JS/template

@ShGKme
Copy link
Contributor

ShGKme commented May 27, 2025

Not sure if this is useful

I'd say if you do not follow styleguide - that's your problem 😀
Component should be imported, used, and have a module with the same single name

@susnux susnux force-pushed the feat/nc-button-deprecations branch from 275c533 to 3924279 Compare May 27, 2025 09:52
@susnux
Copy link
Contributor Author

susnux commented May 27, 2025

@Antreesy I implemented your suggestion, so as you can see from the changed tests now also conditional variants are covered.
@ShGKme native-type is now always migrated.

@susnux susnux force-pushed the feat/nc-button-deprecations branch 2 times, most recently from d859a9e to e97532a Compare May 27, 2025 21:19
@susnux susnux requested a review from ShGKme May 27, 2025 21:20
@Antreesy
Copy link
Contributor

Antreesy commented May 28, 2025

New approach fails at Talk:

TypeError: Cannot read properties of null (reading 'name')
Occurred while linting /home/user/nextcloud-docker-dev/workspace/server/apps-extra/spreed/src/components/AdminSettings/RecordingServer.vue:53
Rule: "vue/no-child-content"
    at file:///home/user/nextcloud-docker-dev/workspace/server/apps-extra/spreed/node_modules/@nextcloud/eslint-config/dist/plugins/nextcloud-vue/rules/no-deprecated-props.js:19:80
    at Array.find (<anonymous>)
    at VElement[name="ncbutton"] VAttribute:has(VIdentifier[name="type"]) (file:///home/user/nextcloud-docker-dev/workspace/server/apps-extra/spreed/node_modules/@nextcloud/eslint-config/dist/plugins/nextcloud-vue/rules/no-deprecated-props.js:18:62)

Failing button:

		<NcButton v-show="!loading"
			type="tertiary"
			:title="t('spreed', 'Delete this server')"
			:aria-label="t('spreed', 'Delete this server')"
			@click="removeServer">
			<template #icon>
				<IconDelete :size="20" />
			</template>
		</NcButton>

@susnux susnux force-pushed the feat/nc-button-deprecations branch 2 times, most recently from 3ce2d5c to 47bad4c Compare May 28, 2025 10:13
@susnux
Copy link
Contributor Author

susnux commented May 28, 2025

Failing button

Thats v-show which is a directive key without argument.
Fixed that.

@susnux susnux force-pushed the feat/nc-button-deprecations branch from 47bad4c to 4d28fd3 Compare May 28, 2025 10:18
@susnux susnux merged commit fb8f8d9 into main May 28, 2025
10 checks passed
@susnux susnux deleted the feat/nc-button-deprecations branch May 28, 2025 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants