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.

Sign up for GitHub

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

Adding a custom lint rule to ensure we use v-clean-tooltip instead of v-tooltip directives. #12633

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Adding a custom lint rule to ensure we use v-clean-tooltip instead of v-tooltip directives. #12633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .eslintrc.default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,10 @@ module.exports = {
'vue/one-component-per-file': 'off',
'vue/no-deprecated-slot-attribute': 'off',
'vue/require-explicit-emits': 'error',
'vue/v-on-event-hyphenation': 'off'
'vue/v-on-event-hyphenation': 'off',

// Locally defined rules, you can find these defined in the `eslint-local-rules` directory.
'v-clean-tooltip': 'error',
},
overrides: [
{
Expand Down
33 changes: 33 additions & 0 deletions eslint-local-rules/v-clean-tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Currently loading these rules with the --rulesdir argument. In the future we could make use of `eslint-plugin-local-rules`.
const vueUtils = require('eslint-plugin-vue/lib/utils');

module.exports = {
meta: {
type: 'problem',
docs: { description: 'We want to use `v-clean-tooltip` instead of `v-tooltip` in most all areas to avoid XSS exploits.' },
schema: [],
},
create(context) {
return vueUtils.defineTemplateBodyVisitor(context, {
VAttribute(node) {
// v-tooltip is a VDirectiveKey
if (node?.key?.type !== 'VDirectiveKey') {
return;
}

// v-tooltip is also a VIdentifier
if (node.key.name.type !== 'VIdentifier') {
return;
}

if (node.key.name.name === 'tooltip') {
context.report({
node: node.key,
loc: node.loc,
message: 'We want to use `v-clean-tooltip` instead of `v-tooltip` in most all areas to avoid XSS exploits.'
});
}
}
});
}
};
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"serve-pkgs": "./shell/scripts/serve-pkgs",
"publish-shell-reset-reg": "cd shell && npm publish",
"clean": "./shell/scripts/clean",
"lint": "./node_modules/.bin/eslint --max-warnings 0 --ext .js,.ts,.vue .",
"lint": "./node_modules/.bin/eslint --rulesdir ./eslint-local-rules --max-warnings 0 --ext .js,.ts,.vue .",
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if we can specify rulesdir in eslintrc instead of with a cli flag?

My assumption is that it's best to stick with the approach in this PR, but my curiousity was piqued when I found some old conversations that suggests eslint deprecating the usage of ruledir and recommending eslint-plugin-rulesdir as the preferred approach12. I'm guessing that this never materialized in any meaningful way, seeing how we are using rulesdir today.

Footnotes

  1. https://github.com/eslint/eslint/issues/2715 ↩

  2. https://github.com/eslint/eslint/issues/8769 ↩

Copy link
Contributor Author

@codyrancher codyrancher Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure. I'll give it a try.

I will note that I also came across rulesdir being deprecated and that this version using --rulesdir was going to be easiest to port when we upgrade eslint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot specify rulesdir in eslintrc.

It failed with
image

Which lead me to go look at the config-validator code and there isn't a rulesdir option.
https://github.com/eslint/eslintrc/blob/v0.4.3/conf/config-schema.js

When we upgrade eslint we will likely want to switch from --rulesdir to --plugin which will cause us to create one extra file to create the plugin with all of our rules. https://eslint.org/docs/latest/use/command-line-interface#--plugin

"lint:lib": "cd pkg/rancher-components && yarn lint",
"lint-l10n": "./node_modules/.bin/yamllint ./shell/assets/translations",
"test": "NODE_OPTIONS=--max_old_space_size=8192 jest --watch",
Expand Down
4 changes: 2 additions & 2 deletions pkg/rancher-components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
],
"types": "./types/index.d.ts",
"scripts": {
"build:lib": "vue-cli-service build --target lib --name @rancher/components src/main.ts",
"lint": "vue-cli-service lint"
"build:lib": "vue-cli-service build --skip-plugins eslint --target lib --name @rancher/components src/main.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the CI that ran on this PR you can see that we already do a separate lint run https://github.com/rancher/dashboard/actions/runs/11963269493/job/33353388233?pr=12633

The lint run that was done at build time would fail because it seemed to change the working directory and couldn't find the new custom lint rules. I didn't see a point in running the lint rules twice.

"lint": "vue-cli-service lint --rulesdir ../../eslint-local-rules"
},
"engines": {
"node": ">=20.0.0"
Expand Down
Loading