Fixing an issue with eslint not working in vscode #12746

codyrancher · 2024-12-03T01:02:29Z

Using rulesdir in the package scripts made it so the rules were only present when running them in the cli and showed errors in editors. This now allows the rules to run in the editor since I made a plugin using a local dependency
I also made is so lint rules can be run from root on pkg/rancher-components

Fixes #12670

Summary

Fixes #

Occurred changes and/or fixed issues

Technical notes summary

Areas or cases that should be tested

Areas which could experience regressions

Screenshot/Video

Checklist

The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
The PR has a Milestone
The PR template has been filled out
The PR has been self reviewed
The PR has a reviewer assigned
The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

- Using rulesdir in the package scripts made it so the rules were only present when running them in the cli and showed errors in editors. This now allows the rules to run in the editor since I made a plugin using a local dependency - I also made is so lint rules can be run from root on pkg/rancher-components Fixes rancher#12670

codyrancher · 2024-12-03T04:38:34Z

pkg/rancher-components/src/components/Form/LabeledInput/LabeledInput.vue

@@ -116,7 +118,7 @@ export default defineComponent({
      isDisabled,
      validationMessage,
      requiredField
-    } = useLabeledFormElement(props, emit);
+    } = useLabeledFormElement(props, emit as emitMethodType);


Adding the emits will change the type of emit passed to the setup method. In this case it will be a union type of 'change' | 'update:value' | 'blur' | 'update:validation' which surprisingly doesn't match the string type expected by useLabeledFormElement. So I extracted the type and then cast appropriately here. I couldn't figure out a generic type that would work.

codyrancher · 2024-12-03T04:39:53Z

pkg/rancher-components/src/components/StringList/StringList.test.ts

@@ -297,7 +297,7 @@ describe('stringList.vue', () => {
      await inputField.trigger('keydown.enter');
      await wrapper.vm.$nextTick();

-      const emitted = (wrapper.emitted('change') || [])[1][0][0];
+      const emitted = (wrapper.emitted('change') || [])[0][0][0];


When emits is specified the value returned by emitted() omits the event type so I had to go through and subtract 1 from each index to account for the lack of event type.

This is a good development. This behavior was quite confusing while we were fixing the unit tests and it was unclear that omitting emits options was the root cause.

codyrancher · 2024-12-03T04:40:28Z

pkg/rancher-components/types/index.d.ts

The lint rules in root require us to have an ending black newline at the end of each file.

codyrancher · 2024-12-03T04:41:01Z

shell/composables/useLabeledFormElement.ts

@@ -70,7 +70,9 @@ export const labeledFormElementProps = {
  }
 };

-export const useLabeledFormElement = (props: LabeledFormElementProps, emit: (event: string, ...args: any[]) => void): UseLabeledFormElement => {
+export type emitMethodType = (event: string, ...args: any[]) => void;


Explained above, I needed to be able to type case the emit method elsewhere so I extracted the definition.

codyrancher · 2024-12-03T04:42:00Z

package.json

@@ -200,7 +200,8 @@
    "webpack-virtual-modules": "0.4.3",
    "worker-loader": "3.0.8",
    "yaml-lint": "1.7.0",
-    "yarn": "1.22.18"
+    "yarn": "1.22.18",
+    "eslint-plugin-local-rules": "link:./eslint-plugin-local-rules"


We make the local rules a linked dev dependency. This allows eslint to search for the rules as a plugin.

We might want to consider documenting this somewhere. I can see how someone might encounter this link in the future and have questions about why we are doing this.

rak-phillip

This looks great - I have one suggestion about typing emits options in composables.

rak-phillip · 2024-12-03T15:34:57Z

pkg/rancher-components/package.json

@@ -48,7 +48,8 @@
    "sass-loader": "~12.0.0",
    "typescript": "4.5.5",
    "vue": "~3.2.13",
-    "vue-template-compiler": "2.7.16"
+    "vue-template-compiler": "2.7.16",


vue-template-compiler is very odd.. I've created an issue to address this and other dependencies that might not be needed after the Vue3 migration #12751

rak-phillip · 2024-12-03T16:22:14Z

shell/composables/useLabeledFormElement.ts

+export type emitMethodType = (event: string, ...args: any[]) => void;
+
+export const useLabeledFormElement = (props: LabeledFormElementProps, emit: emitMethodType): UseLabeledFormElement => {


I think that we might be able to more explicitly type the emits by using defineEmits:

Suggested change

export type emitMethodType = (event: string, ...args: any[]) => void;

export const useLabeledFormElement = (props: LabeledFormElementProps, emit: emitMethodType): UseLabeledFormElement => {

const labeledFormElementEmits = defineEmits(['update:validation']);

export const useLabeledFormElement = (props: LabeledFormElementProps, emit: typeof labeledFormElementEmits): UseLabeledFormElement => {

This should accomplish two things:

Simplify consumption of useLabeledFormElement by removing the requirement to type emits as emitMethodType

Enforces that consumers define update:validation in their emits

Added, I had to also add the defineEmits import and remove the emitMethodType in the other file so I added it as a part of my commit instead of committing the suggestion.

rak-phillip · 2024-12-03T16:24:27Z

pkg/rancher-components/src/components/StringList/StringList.test.ts

@@ -297,7 +297,7 @@ describe('stringList.vue', () => {
      await inputField.trigger('keydown.enter');
      await wrapper.vm.$nextTick();

-      const emitted = (wrapper.emitted('change') || [])[1][0][0];
+      const emitted = (wrapper.emitted('change') || [])[0][0][0];


This is a good development. This behavior was quite confusing while we were fixing the unit tests and it was unclear that omitting emits options was the root cause.

rak-phillip · 2024-12-03T16:25:41Z

package.json

@@ -200,7 +200,8 @@
    "webpack-virtual-modules": "0.4.3",
    "worker-loader": "3.0.8",
    "yaml-lint": "1.7.0",
-    "yarn": "1.22.18"
+    "yarn": "1.22.18",
+    "eslint-plugin-local-rules": "link:./eslint-plugin-local-rules"


We might want to consider documenting this somewhere. I can see how someone might encounter this link in the future and have questions about why we are doing this.

codyrancher · 2024-12-03T23:18:26Z

Added the link documentation to eslint-plugin-local-rules/index.js

rak-phillip

LGTM

rancher-ui-project-bot bot assigned codyrancher Dec 3, 2024

codyrancher force-pushed the lint-fix-2 branch 5 times, most recently from 12e3e89 to c05f3ca Compare December 3, 2024 04:05

codyrancher added this to the v2.11.0 milestone Dec 3, 2024

codyrancher commented Dec 3, 2024

View reviewed changes

codyrancher requested a review from rak-phillip December 3, 2024 04:42

codyrancher marked this pull request as ready for review December 3, 2024 04:42

rak-phillip reviewed Dec 3, 2024

View reviewed changes

codyrancher force-pushed the lint-fix-2 branch from c05f3ca to 8625190 Compare December 3, 2024 23:03

codyrancher requested a review from rak-phillip December 3, 2024 23:48

rak-phillip previously approved these changes Dec 4, 2024

View reviewed changes

Adding emits to all rancher-components to fix linting rules

c77ee88

codyrancher dismissed rak-phillip’s stale review via c77ee88 December 4, 2024 16:06

codyrancher force-pushed the lint-fix-2 branch from 8625190 to c77ee88 Compare December 4, 2024 16:06

rak-phillip approved these changes Dec 4, 2024

View reviewed changes

codyrancher merged commit 6c170fc into rancher:master Dec 4, 2024
33 checks passed

codyrancher deleted the lint-fix-2 branch December 4, 2024 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing an issue with eslint not working in vscode #12746

Fixing an issue with eslint not working in vscode #12746

codyrancher commented Dec 3, 2024 •

edited

Loading

codyrancher Dec 3, 2024 •

edited

Loading

codyrancher Dec 3, 2024

rak-phillip Dec 3, 2024

codyrancher Dec 3, 2024

codyrancher Dec 3, 2024

codyrancher Dec 3, 2024

rak-phillip Dec 3, 2024

rak-phillip left a comment

rak-phillip Dec 3, 2024

rak-phillip Dec 3, 2024

codyrancher Dec 3, 2024

rak-phillip Dec 3, 2024

rak-phillip Dec 3, 2024

codyrancher commented Dec 3, 2024

rak-phillip left a comment

		export type emitMethodType = (event: string, ...args: any[]) => void;

		export const useLabeledFormElement = (props: LabeledFormElementProps, emit: emitMethodType): UseLabeledFormElement => {

Fixing an issue with eslint not working in vscode #12746

Fixing an issue with eslint not working in vscode #12746

Conversation

codyrancher commented Dec 3, 2024 • edited Loading

Summary

Occurred changes and/or fixed issues

Technical notes summary

Areas or cases that should be tested

Areas which could experience regressions

Screenshot/Video

Checklist

codyrancher Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rak-phillip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher commented Dec 3, 2024

rak-phillip left a comment

Choose a reason for hiding this comment

codyrancher commented Dec 3, 2024 •

edited

Loading

codyrancher Dec 3, 2024 •

edited

Loading