Updating <router-link> to no longer use the deprecated :tag property #10793

codyrancher · 2024-04-11T23:16:32Z

Summary

Updating to no longer use the deprecated :tag property

Areas which could experience regressions

Navigation

Screenshot/Video

Demonstrating that the different types of links around the dashboard app still function

router-link-warning.mp4

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

codyrancher · 2024-04-16T17:47:49Z

shell/components/TabbedLinks/index.vue

This file wasn't used anywhere in dashboard.

Are you sure it's not also in other plugins?

It can't be guaranteed but it's a pretty reasonable assumption given it was old, wasn't used in the app as an example, wasn't documented and I couldn't find any shell (@shell/components/TabbedLinks) imports in github.

codyrancher · 2024-04-16T17:48:23Z

shell/components/nav/Type.vue

  },

  computed: {
-    isCurrent() {


This wasn't used in the file.

codyrancher · 2024-04-16T17:48:41Z

shell/components/nav/Type.vue

-      over:     false,
-      menuPath: this.type.route ? this.$router.resolve(this.type.route)?.route?.path : undefined,


These weren't used.

codyrancher · 2024-04-16T17:49:00Z

shell/components/nav/Type.vue

@@ -28,65 +28,10 @@ export default {
  },

  data() {
-    return {
-      near:     false,


near really should be mouseover it seems.

codyrancher · 2024-04-16T17:49:22Z

shell/components/nav/Type.vue

-    setOver(val) {
-      this.over = val;
-    },
-
-    removeFavorite() {
-      this.$store.dispatch('type-map/removeFavorite', this.type.name);
-    },


codyrancher · 2024-04-16T17:52:12Z

shell/components/nav/Type.vue

-      @click="selectType"
-      @mouseenter="setNear(true)"
-      @mouseleave="setNear(false)"


selectType is specific to types in our app while this is being treated as a link to an external page based on the rel="..".

setNear is only actually used in relation to showing the Favorite component which isn't possible in this link.

codyrancher · 2024-04-16T17:56:46Z

shell/components/nav/__tests__/Type.test.ts

The original tests here were mostly exercising and testing router-link instead of our implementation of the Type component.

The new slot API makes these tests non-functional because the RouterLinkStub doesn't fully support the new API. As such I decided to rewrite these tests to achieve the following:

Test the Type component instead of RouterLink

Test more of the functionality than was tested before

Switch to shallowMount instead of mount

More tests could still be written but I think this puts things in a better place than things were. I do recognize that rewriting broken tests is a bit of a code smell though.

codyrancher · 2024-04-16T17:58:04Z

shell/utils/unit-tests/ChildRenderingRouterLinkStub.ts

It's in the comments of this file but I'd like to further point out that this is a workaround because the vue/test-utils RouterLinkStub doesn't actually support the new slot api yet. I took the idea from vuejs/vue-test-utils#1803 (comment).

cnotv

I can see a lot of changes that are unrelated to the : tag property.
May I ask you what are the changes required due to that?

shell/components/nav/__tests__/Type.test.ts

cnotv · 2024-04-17T16:05:06Z

shell/components/nav/__tests__/Type.test.ts

+import { createChildRenderingRouterLinkStub } from '@shell/utils/unit-tests/ChildRenderingRouterLinkStub';
+import { TYPE_MODES } from '@shell/store/type-map';
+
+// Configuration


Could we avoid having data outside the tests? It is very time-consuming to have all this list and try to figure out which test it belongs.

This is usually a code smell pointing to a convoluted structure of the unit tests.
Specifically if in your test you are addressing some common points, they must be shared in the same describe(). Therefore you may not need to repeat assignments or you can scope them to make it more readable.

I usually avoid comments like this and aim to deliver tests, but this one really took me too long, I can see already a lot of headaches in the migration phase.

I think what you're suggesting is that we narrow the scope of the shared data, I think that's fair. I've gone ahead and I've done that.

I extended a pattern which initially defined state outside of the tests ece9a33#diff-8c1a0fb993ec56591f5c80663d5ccf84607dee19981b5bc6ff54064fd23676edR8 I could see how you would see that as different but in my mind the pattern is consistent.

I usually avoid comments like this and aim to deliver tests, but this one really took me too long, I can see already a lot of headaches in the migration phase.

Sorry if I'm misinterpreting this but given the context of the rest of this comment I think you're being impolite here. If you're frustrated with me and you'd like to discuss something, we can. If you're thinking I'm incompetent and don't want me working on vue3 or some other aspect of the app please let me and our managers know.

No it's not about you. I have a personal issue about this which I am trying to address, so I apologize I have caused pressure on you and made you feel uncomfortable. I unfortunately sometimes do not realize it.
I hope you can understand and accept my apologies and also thanks for letting me know.
Just to be clear, it was not something aimed at you but at how I usually do reviews in general.

I will try to be more careful in the future.

shell/components/nav/__tests__/Type.test.ts

shell/utils/unit-tests/ChildRenderingRouterLinkStub.ts

rak-phillip

Functionally, I think this looks good and the refactoring aligns with recommendations for addressing the deprecated tag prop¹

https://router.vuejs.org/guide/migration/#Removal-of-event-and-tag-props-in-router-link- ↩

cnotv · 2024-04-18T08:58:17Z

I can see a lot of changes that are unrelated to the : tag property. May I ask you what are the changes required due to that?

Thanks @rak-phillip got it now 😄

codyrancher · 2024-04-18T11:21:51Z

I can see a lot of changes that are unrelated to the : tag property.
May I ask you what are the changes required due to that?

This questions seems odd to me because I don't think this change meanders very far from just the :tag property. But a summary is:

For each file that used :tag we need to switch to the slot api
- First follow https://eslint.vuejs.org/rules/no-deprecated-router-link-tag-prop
- Second, those docs were out of date and I also had to add the custom attribute to each area
- Those changes would have to happen to these files
  - shell/components/SideNav.vue
  - shell/components/TabbedLinks/index.vue
    - Do we want to make changes here even though the file isn't used anywhere? That seemed like wasted effort to me.
  - shell/components/nav/Header.vue
  - shell/components/nav/Type.vue
After making those changes we also need to update the Type.tests.ts because they broke.
- The lack of support for slots in RouterLinkStub makes all of the original tests useless because ultimately it would come down to what slot props get passed. We could either delete those tests or rewrite them to exercise the Type component.

What's notably missing from that list is the dead code that I deleted from Type.vue. That wasn't necessary but since I was testing the component it seemed like a good time to delete dead code that didn't need to be exercised by a test.

codyrancher · 2024-04-18T11:24:42Z

@cnotv I know @rak-phillip already approved this but I think we need to get through the feedback on this and hash things out otherwise going forward with vue 3 is going to be rough.

cnotv · 2024-04-18T14:40:30Z

@cnotv I know @rak-phillip already approved this but I think we need to get through the feedback on this and hash things out otherwise going forward with vue 3 is going to be rough.

I think it would be sufficient for one of us to approve the PR to proceed. Sometimes I may be overzealous or confused on some points, so I will try to be more careful from my side later on. If there are some other points which you were thinking about, let's have a chat about it, with the hope I can be helpful.

cnotv

LGTM

github-actions bot assigned codyrancher Apr 11, 2024

codyrancher force-pushed the router-link-tag branch from c3524c2 to 0851f02 Compare April 16, 2024 16:57

codyrancher added this to the v2.9.0 milestone Apr 16, 2024

codyrancher commented Apr 16, 2024

View reviewed changes

codyrancher requested a review from rak-phillip April 16, 2024 17:58

codyrancher marked this pull request as ready for review April 16, 2024 17:58

codyrancher requested a review from cnotv April 17, 2024 15:48

cnotv requested changes Apr 17, 2024

View reviewed changes

rak-phillip previously approved these changes Apr 17, 2024

View reviewed changes

codyrancher dismissed rak-phillip’s stale review via e21f0ca April 18, 2024 10:42

codyrancher force-pushed the router-link-tag branch from 0851f02 to e21f0ca Compare April 18, 2024 10:42

Updating <router-link> to no longer use the deprecated :tag property

e205b56

codyrancher force-pushed the router-link-tag branch from e21f0ca to e205b56 Compare April 18, 2024 11:23

cnotv self-requested a review April 18, 2024 14:40

cnotv approved these changes Apr 18, 2024

View reviewed changes

codyrancher merged commit e09df1d into rancher:master Apr 18, 2024
25 checks passed

richard-cox mentioned this pull request Apr 24, 2024

Resource search causes UI to reload #10869

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating <router-link> to no longer use the deprecated :tag property #10793

Updating <router-link> to no longer use the deprecated :tag property #10793

codyrancher commented Apr 11, 2024 •

edited

Loading

codyrancher Apr 16, 2024

cnotv Apr 17, 2024

codyrancher Apr 17, 2024 •

edited

Loading

codyrancher Apr 16, 2024

codyrancher Apr 16, 2024

codyrancher Apr 16, 2024

codyrancher Apr 16, 2024

codyrancher Apr 16, 2024

codyrancher Apr 16, 2024

codyrancher Apr 16, 2024

cnotv left a comment

cnotv Apr 17, 2024 •

edited

Loading

codyrancher Apr 18, 2024

cnotv Apr 18, 2024

rak-phillip left a comment •

edited

Loading

cnotv commented Apr 18, 2024

codyrancher commented Apr 18, 2024

codyrancher commented Apr 18, 2024

cnotv commented Apr 18, 2024

cnotv left a comment

		over: false,
		menuPath: this.type.route ? this.$router.resolve(this.type.route)?.route?.path : undefined,

Updating <router-link> to no longer use the deprecated :tag property #10793

Updating <router-link> to no longer use the deprecated :tag property #10793

Conversation

codyrancher commented Apr 11, 2024 • edited Loading

Summary

Areas which could experience regressions

Screenshot/Video

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher Apr 17, 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

Choose a reason for hiding this comment

cnotv left a comment

Choose a reason for hiding this comment

cnotv Apr 17, 2024 • edited Loading

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 • edited Loading

Choose a reason for hiding this comment

Footnotes

cnotv commented Apr 18, 2024

codyrancher commented Apr 18, 2024

codyrancher commented Apr 18, 2024

cnotv commented Apr 18, 2024

cnotv left a comment

Choose a reason for hiding this comment

codyrancher commented Apr 11, 2024 •

edited

Loading

codyrancher Apr 17, 2024 •

edited

Loading

cnotv Apr 17, 2024 •

edited

Loading

rak-phillip left a comment •

edited

Loading